Subject: FvwmForm/FvwmAnimate problem found!

From: [email-masked] (Paul D. Smith)
Date: 06 Nov 1998 17:01:39 -0500

OK, I found out what's going on between FvwmAnimate and FvwmForm.  It's
a gross bug.

To review: FvwmAnimate generates an FvwmForm with configuration
information in it.  Once you say "Apply", it (FvwmForm) generates
FvwmAnimate module commands to set the options you specified.

In my environment, it only sets the first 2 of 5 options.

Here's the problem (kinda obvious in hindsight :-/): FvwmForm writes 5
lines of data to fvwm2's pipe.  Then it exits!!  (You can already see
where this is going, I'm sure :).

On the fvwm2 side, it starts to read the data.  It gets the first two
lines and processes them.  For those unfamiliar with this, the loop
we're talking about is events.c:My_XNextEvent().  It does a select(),
then loops through all the read/write filedescriptors looking for
something with data:

  retval=select(fd_width,&in_fdset, &out_fdset, 0, NULL);
    ...
  for(i=0;i<npipes;i++)
    {
      if(readPipes[i] >= 0)
	{
	  if((retval>0)&&(FD_ISSET(readPipes[i], &in_fdset)))
            ...process read data...

      if(writePipes[i] >= 0)
	{
	  if((retval>0)&&(FD_ISSET(writePipes[i], &out_fdset)))
            ...
       	      FlushQueue(i);

What happens is that there's data on the FvwmForm module's write pipe (I
don't know how it got there; maybe it was a broadcast message or
something--actually I just looked at at least the first item in the
queue is a FOCUS_CHANGE event).

So, after it reads and processes the second line on the FvwmForm read
pipe, it calls FlushQueue() to flush the write pipe queue.  In
module.c:FlushQueue() we see:

  while(pipeQueue[module] != NULL)
    {
      d = pipeQueue[module];
      dptr = (char *)d->data;
      while(d->done < d->size)
	{
	  a = write(writePipes[module],&dptr[d->done], d->size - d->done);

Here it just goes through the data queued for the pipe and tries to
write it.  BUT!  Since the write pipe has been closed by FvwmForm
exiting, write(2) returns an error (-1) and errno is, of course, EPIPE.
In this case, the code run KillModule(), which closes the fvwm2 end of
both the read and write pipes with prejudice, and cleans up everything
else.

And any data left in that read pipe, like the last few lines of the
FvwmForm results, is lost.

:(

This is an important bug and must be fixed.  However, the fix will
require some careful tiptoeing, I think.  Offhand, I see two ways to fix
it:

 1) Change all the modules (that might have write data--maybe this is
    only FvwmForm?) so before exiting they loop on their write pipe
    until it's empty, then exit.

This, I think is a hack but it's probably the fastest way to fix
FvwmForm.  Who knows that other modules may run into this, though?  I
hate these kinds of timing bugs :-/.

 2) Modify fvwm2 so that it doesn't throw out any data in the read pipe
    (close it) just because the write pipe was closed.  I imagine this
    could be done by managing the two pipes independently and only
    tearing down the module information when _BOTH_ the read and write
    pipes were closed.

    In other words, FlushQueue() shouldn't automatically call
    KillModule() if it gets back an error (well, maybe it could but
    KillModule would be more intelligent--that would probably be
    simpler).

    Instead of that, it would close the write side of the pipe (in this
    case), and zap the filedescriptor in writePipe[] so it wouldn't be
    written to or managed any further.

    Then, it would continue on.  At some point during
    HandleModuleInput() the read pipe would be drained, then it, too,
    would return EPIPE during the next read.  KillModule() could be
    invoked again and this time it would see that both the pipes were
    down, and it could tear down the connection for real.

    Ditto for the other way.

    The questions here are what about deadlock?  Can we get into a state
    where one of the pipes stays open indefinitely?  And where, if
    anywhere, in the current code are assumptions made about both pipes
    being available (if one is, the other always is) that might break if
    we changed this.

Anyway, that's too much for me to work on right now, but it's a nice
juicy UNIX-y kinda bug if someone else wants to tackle it :).  If not
we'll have to re-examine it later.

-- 
-------------------------------------------------------------------------------
 Paul D. Smith <[email-masked]>         Network Management Development
 "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
-------------------------------------------------------------------------------
   These are my opinions---Nortel Networks takes no responsibility for them.
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Dan Espen <[email-masked]>
Date: Fri, 06 Nov 1998 17:35:54 -0500

> require some careful tiptoeing, I think.  Offhand, I see two ways to fix

Good find.  I'm not sure why its taken so long to surface, or why
I couldn't reproduce it the next day.

How about if we just have FvwmForm fflush the pipe before exit?

Maybe the flush belongs in the WritePipe routine?

>  Paul D. Smith <[email-masked]>         Network Management Developmen

--
Dan Espen                                Phone: (732) 699-5570
444 Hoes Lane  Room RRC 1C-214           E-mail: [email-masked]
Piscataway, NJ 08854
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: [email-masked] (Paul D. Smith)
Date: 06 Nov 1998 17:59:55 -0500

%% Dan Espen <[email-masked]> writes:

  de> Good find.  I'm not sure why its taken so long to surface, or why
  de> I couldn't reproduce it the next day.

Well, it's pretty timing-dependent, I think.  For example, when I had
FvwmForm compiled with Purify it didn't show up, because Purify was
calculating the memory leaks and didn't really close the app until after
fvwm2 managed to grab all the data.

  de> How about if we just have FvwmForm fflush the pipe before exit?

Won't work; fflush() just pushes any data in the stdio buffers down into
the file (pipe).  It doesn't wait until the reader at the other end of
the pipe has read everything and the pipe is empty again.

Also, fflush() works on a FILE* but fvwm and modules write to pipes
using low-level write(2) and filedescrs, so there's no buffered data in
the first place.

You'd need to select on the write pipe, somehow, and wait until it
cleared.  I _assume_ there's some way to tell but to be honest I'd have
to go look it up to be sure; I don't think I've ever done anything like
that.  This was basically my suggestion #1 :)

---

From: Dan Espen <[email-masked]>
Date: Sat, 07 Nov 1998 08:45:17 -0500

> we'll have to re-examine it later.

Sorry to have  been a little dense there,  this time  I carefully read
what you wrote and checked the code.  I  favor the second fix over the
first because its the right way to go.  The pipe structures aren't too
widely used in the code, and both pipes really  should be empty before
they are torn down.  Note  the comment I  left  in module.h about  the
pipe arrays.

The much simpler fix is to change the  form that FvwmAnimate generates
so  that the apply buttons leave  the form  running.  Only the dismiss
button    will do a    quit.  I've  already   made  this  change to my
development copy until the underlying problem can be addressed.

Another fix would be along the lines of your  first suggestion.  Right
now, when FvwmForm sends commands to FvwmAnimate,  any errors in input
are sent to stderr.  I've toyed with  the idea for making  FvwmForm an
interactive module.  That way, when bad input  is sent to FvwmAnimate,
FvwmAnimate can turn around and send a command back to FvwmForm saying
Field X is invalid.   The same technology would  be used to  shut down
FvwmForm.  Ie.     if all fields   are  valid, FvwmAnimate  would tell
FvwmForm to stop.

---

From: [email-masked] (Paul D. Smith)
Date: 08 Nov 1998 00:42:02 -0500

de> The much simpler fix is to change the  form that FvwmAnimate generates
  de> so  that the apply buttons leave  the form  running.  Only the dismiss
  de> button    will do a    quit.  I've  already   made  this  change to my
  de> development copy until the underlying problem can be addressed.

I agree this is probably the very simplest solution, but of course (a)
it's not foolproof (maybe people can hit the keys to apply/close fast
enough to make it happen?) and (b) as a user-interface issue I much
prefer the old way :)

Anyway, hopefully this bug will be fixed before 2.2 ships but if not is
should be a high priority for the next release.

PS. I agree with some of your comments regarding pipes: it would be nice
    if the data about a module was collected into one structure rather
    than spread across multiple disparate arrays.

    However, I don't think the issue with file descriptors getting large
    is right: the offset into the array isn't the file descriptor (it
    can't be since read and write use different descriptors, but you
    need the same array offset for both so you can correlate them).  So,
    I see no reason that this array ever needs to be larger than the
    number of active modules.

    And, in general, I'd prefer an array over a linked list for this:
    it's faster to search and slightly easier to handle looping.  Of
    course, you have to look for and re-use dead spots (and skip them)
    whereas with a linked list you can just remove any "dead
    spots"... well, six of one.  The time difference probably wouldn't
    be that large.  But I still lean towards keeping the array (but make
    it an array of "module structures" containing read/write pipe fd's,
    name, etc.)