Subject: double FvwmAuto: fvwm hangs in read()

From: Dominik Vogt <[email-masked]>
Date: Sun, 23 Apr 2000 15:14:26 +0200

I've spent some time debugging the hang when two FvwmAutos are
started.  It seems to happen when they actually try to raise a
window.  All locking (UNLOCK messages) seems to work ok.  But
then, fvwm hangs in a read() call in events.c (line 2361):

  FD_ZERO(&in_fdset);
  FD_SET(x_fd,&in_fdset);
  /* nothing is done here if fvem was compiled without session support */
  if (sm_fd >= 0)
    FD_SET(sm_fd, &in_fdset);
  FD_ZERO(&out_fdset);
  for(i=0; i<npipes; i++) {
    if(readPipes[i]>=0)
      FD_SET(readPipes[i], &in_fdset);
    if(pipeQueue[i]!= NULL)
      FD_SET(writePipes[i], &out_fdset);
  }

  DBUG("My_XNextEvent","waiting for module input/output");
  XFlush(dpy);
  if (fvwmSelect(fd_width, &in_fdset, &out_fdset, 0, timeoutP) > 0) {

    /* Check for module input. */
    for (i=0; i<npipes; i++) {
      if ((readPipes[i] >= 0) && FD_ISSET(readPipes[i], &in_fdset)) {
-->     if (read(readPipes[i], &targetWindow, sizeof(Window)) > 0) {
          DBUG("My_XNextEvent","calling HandleModuleInput");
          HandleModuleInput(targetWindow, i, NULL);
    } else {
          DBUG("My_XNextEvent","calling KillModule");
          KillModule(i);
        }
      }
      if ((writePipes[i] >= 0) && FD_ISSET(writePipes[i], &out_fdset)) {
        DBUG("My_XNextEvent","calling FlushQueue");
        FlushQueue(i);
      }
    }

So, since the file descriptor has changed, the only explanation I
can think of is that less than sizeof(Window) bytes are available
on the input pipe.  Does this make sense?

Since every packet sent by a module contains a 'window' entry
(sizeof(Window)), there are several explanations:

 1) The module sent trash.
 2) Part of the input has already been read somewhere else.
 3) fvwmSelect lies about the state of the file descriptor.

All of this seems very unlikely to me.  Any better ideas?

Bye

Dominik ^_^

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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: Chris Rankin <[email-masked]>
Date: Tue, 25 Apr 2000 10:38:46 +1000 (EST)

> -->     if (read(readPipes[i], &targetWindow, sizeof(Window)) > 0) {

Is it possible that the same descriptor is appearing twice in the
readPipes[] array? This would mean that the first read would empty the
pipe and the second would then block.

Chris.

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 26 Apr 2000 17:49:33 +0200

> pipe and the second would then block.

No, that's not what happens (I checked it with a few fprintf's).
The two instances of FvwmAuto have the module numbers 6 and 7 in
my setup and the read pipe numbers 18 and 20.  The last thing I
see before fvwm blocks is that the select call returns with both
pipes (18 and 20) changed.  The read() call on pipe 18 returns
successfully, but the read() call on pipe 20 blocks for no
apparent reason.

---------- one hour later ----------

Okay, this is what happens:

0) Both FvwmAutos (I'll call them FA1 and FA2 from now)
   deciade at the same time that the focused window must be
   raised.  Both put a 'silent raise' into their pipe.
1) Fvwm enters the module input loop in events.c.
2) Fvwm reads 'silent raise' from FA1's pipe.
3) Fvwm raises the window and puts a M_RAISE_WINDOW packet
   into FA1's pipe.  This type of packet is in FvwmAuto's
   sync mask, thus fvwm waits for an 'UNLOCK' response from
   FA1.
4) FA1 responds with 'UNLOCK'.
5) Fvwm sends the M_RAISE_WINDOW to FA2 and waits for 'UNLOCK'.
6) Fvwm is now in the loop in module_interface.c, lines 1205
   to 1272.
7) FA2 puts 'UNLOCK' into the pipe.
8) Fvwm reads 'silent raise' from FA2's pipe before it can get
   the 'UNLOCK':
     while (HandleModuleInput(targetWindow, module, NULL) != 66);
9) Again, fvwm sends M_RAISE_WINDOW packet to FA1 and gets
   'UNLOCK'.
10) Fvwm sends M_RAISE_WINDOW packet to FA2 and gets the old
    'UNLOCK' that was already queued.  FA2 puts another
    'UNLOCK' into the pipe.
11) Fvwm returns from the HandleModuleInput() call in step 8.
12) In the next pass of the loop, Fvwm reads the expected
    'UNLOCK' from FA2's queue.
13) Fvwm breaks the loop and returns to the loop in events.c.
14) Fvwm tries to read from FA2's queue which is empty now.

==> BANG

When the select call returned that FA2's filedescriptor changed
in the first place, this was because of the 'Silent Raise'. But
this string was eaten up while FA1's input was processed (step 8).
So when fvwm finally got around to read from FA2's pipe, nothing
was left to be read.

One simple but inefficient fix comes to my mind: select()
the module input pipes one by one, not in one big loop.

Another idea:  why do the read pipes block?  The code
in events.c:

  if (read(readPipes[i], &targetWindow, sizeof(Window)) > 0) {
    DBUG("My_XNextEvent","calling HandleModuleInput");
    HandleModuleInput(targetWindow, i, NULL);
  } else {
    DBUG("My_XNextEvent","calling KillModule");
    KillModule(i);
  }

could be rewritten like this:

  if (read(readPipes[i], &targetWindow, sizeof(Window)) > 0) {
    DBUG("My_XNextEvent","calling HandleModuleInput");
    HandleModuleInput(targetWindow, i, NULL);
  } else if (/*error condition*/) {
    DBUG("My_XNextEvent","calling KillModule");
    KillModule(i);
  } else {
    /* nothing read, ignore */
  }

There are several places in module_interface.c that would have
to be changed too.

Chris, can you do something about it?

Bye

Dominik ^_^

---

From: [email-masked] (Paul D. Smith)
Date: 26 Apr 2000 15:08:27 -0400

%% Dominik Vogt <[email-masked]> writes:

  dv>      while (HandleModuleInput(targetWindow, module, NULL) != 66);

Ulp.  Can we remove magic numbers, please?

  dv> ==> BANG

Well, first I'd like to point out that it's silly (isn't it?) to have
two FvwmAuto modules running, and maybe the thing to do is make it so
that can't happen.

Can a module find out what other modules are running?  I know I should
just go look :-/.  It seems like it would be a useful thing to provide
modules the ability to ask for a list of information about registered
modules.  If so, when FvwmAuto first starts it could ask for this list,
check if there's a different FvwmAuto, and die if so.

Of course, this situation may be reachable in other ways, in which case
we'd obviously need a more generic solution.

  dv> One simple but inefficient fix comes to my mind: select()
  dv> the module input pipes one by one, not in one big loop.

I don't see how this would work: you'd have to have a loop where you
select with no waiting on each read FD at a time, then loop back to the
beginning and start over.  But that's a hard loop and will cause FVWM to
eat all the CPU!  Or, you'd have to give each select a short timeout,
but then the responsiveness of your modules goes incrementally down as
you add more modules, even relatively inactive ones, since each one gets
a little timeslice (and of course, your CPU usage for FVWM still goes
up).  Or, you'd select with no wait, then sleep a little, then start
over, but now you're slowing down all modules (and CPU usage is still
higher).

I think this should be avoided if possible.

  dv> Another idea:  why do the read pipes block?

Well, the harder part is setting the FD for non-blocking reads in the
first place.  Unfortunately the method for doing that isn't as portable
as you might hope, IIRC... I think some older systems can't even do it.
You'd probably need some autoconf support for this.  I played with this
a good bit a couple of years ago in GNU make, I'll see if I can remember
the details.

-- 
-------------------------------------------------------------------------------
 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.fvwm.org/>.
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: Mikhael Goikhman <[email-masked]>
Date: Wed, 26 Apr 2000 22:23:00 +0300

> check if there's a different FvwmAuto, and die if so.

Ability to check for module uniqueness is good, but should be optional.
Say, using UniqueModule command. All current "event" modules (FvwmAuto,
FvwmBanner, FvwmEvent) are very generic/configurable, i.e. they can be
programmed to execute any arbitrary fvwm function, not only autoraise
windows, or set background images, or play sounds, or all together.

Regards,
Mikhael.

---

From: [email-masked] (Paul D. Smith)
Date: 26 Apr 2000 17:31:29 -0400

%% Mikhael Goikhman <[email-masked]> writes:

  >> check if there's a different FvwmAuto, and die if so.

  mg> Ability to check for module uniqueness is good, but should be optional.
  mg> Say, using UniqueModule command.

No, I meant it would be up to the module to determine what to do.  In
other words, fvwm would merely supply the module list, then the module
would decide what to do with it.

FvwmAuto would check for any other module besides itself (the module
list would need to contain uniquifying info, like maybe the pipe FD
numbers) and die if any such existed.

Other modules might do other things (maybe you could write a "module
manager module" :).

  mg> All current "event" modules (FvwmAuto, FvwmBanner, FvwmEvent) are
  mg> very generic/configurable, i.e. they can be programmed to execute
  mg> any arbitrary fvwm function, not only autoraise windows, or set
  mg> background images, or play sounds, or all together.

FvwmAuto doesn't fit with the rest of the items you list there.  It's
not at all configurable.  I can't see _any_ reason to _ever_ run more
than one.

Unless I'm missing something ...

If there really are reasons to have multiple modules, like for
FvwmEvent, then obviously they wouldn't even bother to check this.

---

From: Chris Rankin <[email-masked]>
Date: Thu, 27 Apr 2000 08:18:37 +1000 (EST)

> Chris, can you do something about it?

I'll have a look, but won't get a chance to do anything for a few
days. That leaves plenty of time for us to thrash out the problem :-).

I agree with Paul that it doesn't make much sense to have two
FvwmAutos running, but won't there be other cases where important
messages will be thrown away like this? I'd prefer to find a deeper
solution to this rather than a "then don't do that" type fix, at this
stage anyway.

Chris

---

From: Mikhael Goikhman <[email-masked]>
Date: Thu, 27 Apr 2000 03:12:40 +0300

> not at all configurable.  I can't see _any_ reason to _ever_ run more

Did you ever read FvwmAuto(1)? :-)
Here are some useful tips.

DestroyFunc RestoreIconified
AddToFunc   RestoreIconified
+ I Current (Iconic) Iconify false

DestroyFunc RegisterFocus
AddToFunc   RegisterFocus
+ I Exec echo "`date +%T` window [$n] focussed" >>/tmp/focus-stats.txt

DestroyFunc RegisterUnfocus
AddToFunc   RegisterUnfocus
+ I Exec echo "`date +%T` window [$n] unfocussed" >>/tmp/focus-stats.txt

Module FvwmAuto 250 Raise Nop
Module FvwmAuto 800 "Function RestoreIconified" Nop
Module FvwmAuto   0 "Function RegisterFocus" "Function RegisterUnfocus"

Unfortunatelly this does not work as expected now, I suppose, because of
syncronization/pipe problems. Try running two FvwmAuto's first.

Regards,
Mikhael.

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 27 Apr 2000 03:56:41 +0200

> two FvwmAuto modules running, and maybe the thing to do is make it so

I think it is.

> we'd obviously need a more generic solution.

See my other mail for a detailed analysis of the problem.

> I think this should be avoided if possible.

Of course you're right, Paul.  The loop would eat up all CPU.  I didn't
mean this 'fix' seriously anyway.

> a good bit a couple of years ago in GNU make, I'll see if I can remember

Then, what can we do?  Is it possible to code this approach:

 1) select() for all pipes
 2) handle input from the first changed pipe
 3) remove the changed pipe and all unchanged ones from the set
 4) select() again on the remaining pipes, return immediately if
    no input is available.
 5) repeat 2-5 until no pipes have input

I'm not sure how select() works exactly.

Bye

Dominik ^_^

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 27 Apr 2000 03:44:13 +0200

> > Another idea:  why do the read pipes block?  The code
...
> solution to this rather than a "then don't do that" type fix, at this

This can happen with other (different) modules too.  Fvwm will
freeze if the following conditions are met:

 1) Two modules send a command at the same time (the commands can
    be different).  The important part is that the select() call
    will report that both module pipes have changed.

     AND

 2) The command from the first module triggers a message from fvwm
    to the second module that is selected by the module's
    SyncMask.

The following modules currently use the SyncMask:

  FvwmAnimate  M_(DE)ICONIFY, M_STRING
  FvwmAuto     M_FOCUS_CHANGE, M_RAISE_WINDOW, M_LOWER_WINDOW
  FvwmGtk      M_STRING
  FvwmIconBox  M_(DE)ICONIFY
  FvwmIconMan  M_(DE)ICONIFY
  FvwmTaskBar  M_(DE)ICONIFY
  FvwmWinList  M_(DE)ICONIFY

As far as I know, only FvwmAuto sends anything to fvwm not
directly caused by user interaction.  Condition (2) can
be fulfilled easily, e.g. by iconifying a window.  The
difficult part is to have input from another module at the
same time.  FvwmConsole may be a good candidate to force
this situation:

 - Paste many lines like 'next iconify toggle' into the
   FvwmConsole window.
 - At the same time repeatedly click on a button in FvwmIconMan
   fast.

Hold on, I'll give it a try...

--------- 5 minutes later ---------

Yup, I get the same problem.  What I did is this:

 - KillModule *
   (to make sure the modules are in the correct order)
 - Open an xemacs window and write
     'next (emacs*) iconify toggle'
   1000 times
 - Start FvwmConsole from a menu (it's important that
   FvwmConsole is started first - order matters).
 - Start FvwmIconMan from a menu.
 - Paste the complete xemacs buffer 10 times into the
   console window.
 - While fvwm is busy (de)iconifying, hit any button in
   FvwmIconMan often.  Once in a while stop doing this
   and watch what happens.

==> Sometimes fvwm stops (de)iconifying.  In this
    situation it is frozen until I press a button in the
    IconMan window to feed the read() call.

The problem with FvwmAuto is that there is nothing the user
can do to wake up the idle module because FvwmAuto doesn't
take user input, only input from fvwm - and fvwm is already
frozen.

BTW, a possible 'solution' could be some kind of heartbeat
from the modules.  FvwmAuto could send a 'Nop' every 5
seconds or so to keep fvwm alive :-)

Bye

Dominik ^_^

---

From: Michael Han <[email-masked]>
Date: Thu, 27 Apr 2000 00:29:53 -0700

Previously from Mikhael Goikhman
>[snip]

Based on Dominik's description, this should work, shouldn't it? The
synchronous behavior of multiple FvwmAutos shouldn't be relevant if
all your FvwmAutos are running with different delay timings. Did I
misunderstand?

What exactly is the purpose of having synchronous module behavior here
for focus/raise/lower? I'm sort of interested in pursuing the idea of
integrating FvwmAuto into FvwmEvent -- this would require adding delay
timers to FvwmEvent, and ideally should also include the ability to
define multiple actions per event on different timers. But generally
speaking, FvwmEvent provides a pretty nice generic framework for doing
what FvwmAuto does. It can certainly handle the simplest case
scenario of autoraise with zero delay.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
The life uncaffeinated is not worth living.
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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: Mikhael Goikhman <[email-masked]>
Date: Thu, 27 Apr 2000 14:40:13 +0300

> all your FvwmAutos are running with different delay timings. Did I

Well, after I fixed an allocation bug, this works much better. In fact,
if I replace 800 with 100, all three FvwmAuto's work together well.

In the original form, the second FvwmAuto is ignored, because, I suppose,
of lock-on-raise. FvwmAuto is a bit raise/lower oriented, which makes it
hardier to use for other purposes...

> scenario of autoraise with zero delay.

Extending FvwmEvent would be good. But probably using FvwmEvent aliases
with one timeout and one event action may be prefered than "multiple
actions per event on different timers". Seems easier to implement and use.
Of course, this is not at all important before 2.4.

Regards,
Mikhael.

---

From: Chris Rankin <[email-masked]>
Date: Thu, 27 Apr 2000 23:41:34 +1000 (EST)

>     to the second module that is selected by the module's

OK, I've poked around the code a little and digested Dominik's bug
synopsis. In summary: it looks like the root of the problem is the way
that BroadcastPacket(M_RAISE_WINDOW) calls PositiveWrite() for all
modules. PositiveWrite() expects a reply from each descriptor and
therefore destroys the "read-state" of every descriptor as recorded
earlier in My_XNextEvent().

I'll throw out a few clay pigeons for everyone to shoot at:
- we can either try to preserve My_XNextEvent()'s idea of which
  descriptors are still readable, or we can recognise that certain
  functions will trash this information.

- if we can spot when My_XNextEvent()'s readable descriptor set
  (in_fdset) has been invalidated then we could do something like:

  reset:
  if (fvwmSelect(fd_width, &in_fdset, &out_fdset, 0, timeoutP) > 0) {
    ...
      HandleModuleInput(targetWindow, i, NULL);
      if (descriptorsChanged) goto reset;

  This approach is hopefully low-impact, which is good since we're
  aiming for 2.4.

- Having thought about what I've just written, I am now worried that
  this goto approach might make fvwm neglect pipes in readPipes[i]
  with large "i" in favour of pipes with smaller "i". (Did that make
  sense?) Maybe instead we could just call fvwmSelect() again using
  the existing fd_sets and a timeout of zero? This way, we would only
  check that pipes that we thought had data still do have data. We
  would also have to call FD_CLR() for each descriptor:

    /* Check for module input. */
    for (i=0; i<npipes; i++) {
      if ((readPipes[i] >= 0) && FD_ISSET(readPipes[i], &in_fdset)) {
        FD_CLR(readPipes[i], &in_fdset);

          /* reset our idea of which descriptors have data waiting to
             be read, as necessary ...
             Maybe use return code from HandleModuleInput instead of
             this global "descriptors have changed" variable? */
          if (descriptorsChanged &&
              fvwmSelect(fd_width, &in_fdset, &out_fdset, 0, 0) <= 0) {
            break;
          }

	} else {
          DBUG("My_XNextEvent","calling KillModule");
          KillModule(i);
        }
      }
      if ((writePipes[i] >= 0) && FD_ISSET(writePipes[i], &out_fdset)) {
        FD_CLR(writePipes[i], &out_fdset);

    } /* for */

- is there any way for a pipe to receive "exceptional" data? If we
  could get the UNLOCK to be notified via select()'s fdset *exceptfds
  argument then the read-state would remain intact. (I have very
  little hope here - the man pages imply that this is more of a
  socket-protocol thing.)

- I was going to look for a solution whereby in_fdset (and out_fdset?)
  aren't invalidated in the first place, but that turns very evil very
  quickly by striking at the root of BroadcastPacket and the SyncMask
  functionality ...

What do people think? How many functions would need to be modified to
support the "descriptors have changed" functionality? Or do you think
I'm barking up totally the wrong tree?

Cheers,
Chris.

---

From: Tim Phipps <[email-masked]>
Date: Thu, 27 Apr 2000 15:07:21 +0100

>   little hope here - the man pages imply that this is more of a

I think you've hit the nail on the head.  Something from my dim and
distant networking days is coming back.  It's "out of band" signaling.

Fvwm wants an UNLOCK from every SyncMask message and the current problem
is that it can't get at them until it has processed all the other
messages the module has sent.

The correct solution to this class of problems is for fvwm to wait for
the UNLOCK command without doing anything else and then process any
other module commands in normal order.

We have a choice of ways to do this:

1) don't use the module pipes to pass the UNLOCK message, we could use
an extra pipe (not backwards compatible, extra file descriptors), or
maybe we could use signals.

2) use the module pipes and buffer any commands received until the
UNLOCK, this is similar to the way XLib works, you can look for a
certain event type, other events received are added to the queue and
will be retrieved by the next XNextEvent().

If the signals way is portable and safe I think I'd prefer that, the
sync code could be something like:

	SendMessage();
	SetSig(USR2, interrupt);
	sleep(some_large_timeout);
	SetSig(USR2, ignore);

You could even replace the sleep with a (set cursor to busy, wait for a
timeout or escape key sequence or signal).

Cheers,
Tim.

---

From: Dominik Vogt <[email-masked]>
Date: Thu, 27 Apr 2000 16:48:37 +0200

On Thu, Apr 27, 2000 at 12:29:53AM -0700, Michael Han wrote:
> for focus/raise/lower?

It's necessary to synchronize FvwmAuto and fvwm to avoid race
conditions.  In the past we didn't use the SyncMask for FvwmAuto,
but when the user had Mouse/SloppyFocus, dragging the pointer
across several windows could lead to a random window being raised.
Setting the SyncMask dramatically reduces this risk.

> I'm sort of interested in pursuing the idea of

Well, perhaps we should think about integrating FvwmEvent into
the main module (along with FvwmAuto).  With the current module
interface, asynchronous event handling is a pain and often causes
hard to debug race conditions.  Just a thought.

Bye

Dominik ^_^

---

From: Michael Han <[email-masked]>
Date: Thu, 27 Apr 2000 10:13:59 -0700

Previously from Dominik Vogt
>Setting the SyncMask dramatically reduces this risk.

Ah. I thought it was something along those lines.

>hard to debug race conditions.  Just a thought.

Well... that would involve adding quite a bit of stuff to the core,
wouldn't it? First you need the event handling. Then you need the
"sleep"-type function you get with FvwmAuto.
-- 
[email-masked]                         http://www.best.com/~mikehan/
coffee achiever                              San Francisco, California
Why do we still live here in this repulsive town? All our friends are in New
York." Stephen Merritt
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
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: Mikhael Goikhman <[email-masked]>
Date: Thu, 27 Apr 2000 23:35:12 +0300

> hardier to use for other purposes...

Ok, this is fixed, FvwmAuto now is only raise/lower oriented if needed.
Though this has nothing to do with the original "read pipes" problem.

Regards,
Mikhael.

---

From: Chris Rankin <[email-masked]>
Date: Fri, 28 Apr 2000 08:18:55 +1000 (EST)

> will be retrieved by the next XNextEvent().

Out-of-band messaging would be nice, but I'm not sure how feasible it
is using pipes. Basically, every module would need a second "out of
band" pipe because we would still want to be able to tell which
message was from whom and not have to throw away other messages to
find the one we were interested in. And our solution would need to be
portable too. When I briefly investigated this, it seemed that the
"simplest" way might be to replace pipes with AF_UNIX sockets.

Chris