Re: lynx-dev lynx and ncurses and gpm

[Alexander V. Lukyanov]

On Fri, Apr 23, 1999 at 05:59:18AM -0500, Klaus Weide wrote:
> On Fri, 23 Apr 1999, Alexander V. Lukyanov wrote:
> > On Thu, Apr 22, 1999 at 07:37:37AM -0500, Klaus Weide wrote:
> > > There is a provision to deal with an interrupted read() in fifo_push()
> > > if HIDE_EINTR is defined.  But HIDE_EINTR is explicitly #undef'd in
> > > fifo_defs.h, with no explanation except the 980606 NEWS file entry
> > > (which I assume corresponds to the #undef)
> > > 
> > >         + removed the cycle on EINTR, as it seems to be useless.
> > > 
> > > I don't know how someone came to that conclusion, and disagree with
> > > it (as does the lib_tstp.c comment above).
> > 
> > It is done for purpose. E.g. in case of xterm resize application should
> > act immediately, and EINTR for SIGWINCH helps that. I'm sure applications
> > can want to react to other signals outside of signal handlers (to avoid
> > reentering non-reenterable functions) by setting a flag inside and then
> > checking it in main loop.
> 
> I can see how this helps applications to react to resize events more
> immediately.  So that is the reason why ncurses tries to install its
> own handler for SIGWINCH without SA_RESTART, different from those for
> SIGTSTP, SIGINT, SIGTERM.  I had wondered about that.
> 
> But while it helps some applications, it makes others fail.
> 
> > getch can also return ERR on timeout if one was set.
> 
> Yes, and that is documented in the getch(3) man page.  But the behavior
> you are defending directly *contradicts* the man page:
> 
>  PORTABILITY
>  .....
>        The  behavior of getch and friends in the presence of han-
>        dled signals is unspecified in the  SVr4  and  XSI  Curses
>        documentation.   Under  historical curses implementations,
>        it varied depending  on  whether  the  operating  system's
>        implementation  of  handled  signal  receipt  interrupts a
>        read(2) call in progress or not, and also (in some  imple-
>        mentations)  depending on whether an input timeout or non-
>        blocking mode hsd been set.
> 
>        Programmers concerned about portability should be prepared
>        for  either  of  two  cases:  (a)  signal receipt does not
>        interrupt getch; (b) signal receipt interrupts  getch  and
>        causes  it  to  return ERR with errno set to EINTR.  Under
>        the ncurses implementation, handled signals  never  inter-
>        rupt getch.
> 
> Note the last sentence.  I handn't seen it before I sent my prvious
> message; now I am more convinced that this is a real bug.  If

The manual refers to the old behaviour, it was changed later.
Yes, it needs to be fixed. But there is another interesting sentence:
"return ERR with errno set to EINTR". That can be used to identify the
ERR reason. But still I'm not sure it is portable.

> applications *need* this behavior, they are relying on a bug (if they
> were written to the ncurses interface) or on non-portable behavior.
> They could and should use no-delay or half-delay mode if they really
> want to make sure they get timely notification, or some other means.

Half-delay is possible to use to get the interrupting behavior. (if
ncurses is modified accordingly). But it is still not possible to
know if it is timeout passed or other reason caused getch to return ERR.

> I am also concerned that the rationale you states isn't mentioned
> anywhere (that I could find) - not in the source code, not in NEWS,
> and every piece of comment I *did* find gives the wrong impression
> that ncurses is trying hard to never interrupt getch.

It did for some time, until the problem with screen resize arised.

> > So I beleave applications should not immediately exit on ERR from getch.
> 
> Yes, that is probably good advice.  But according to the existing
> documentation, an application (if it's using ncurses) that doesn't use
> no-delay or half-delay mode has every right to assume that ERR means a
> real failure.

If application relies on that, then it is not portable, as stated in the
man page.

> Btw. what *portable* way do you suggest for programs to determine whether
> an ERR means a real unrecoverable failure?

Unfortunately I don't know one. Likely case is the trick with errno,
but it does not seem very reliable/portable.

> > What is really bad in getch implementation is lack of distinction
> > between ERR as retriable error (such as EINTR or EAGAIN) and hard error
> > like terminal hangup.
> 
> I see you agree there is a problem...
> 
> One partial solution would be adding a function by which the application can
> set the desired behavior.  The applications you have in mind would call this
> to set 'interrupting' behavior (and then would have to check after each ERR
> whether it's a hard error); other more naive applications get the currently
> documented behavior.

I think it will be better to add an xwgetch function (extended getch), which
would remove the ambiguity of ERR. But this would introduce incompatibility
with other curses implementations and old applications would not benefit from 
it.

> [One could argue that this is already achieved by the decision whether to set
> SA_RESTART or not on handlers; but 1. that doesn't work on all systems, and
> 2. not all signal handlers are under the application's or ncurses' control
> as demonstrated in the gpm case, and 3. a separate function to control global
> behavior would allow easy switching between the two modes.]
> 
> For the WINCH case, there already is a mechanism for timely notification,
> added by yourself; from NEWS:
> 
>   970906
>   .....
>         > patches by Alexander V. Lukyanov:
>         + add pseudo-functionkey KEY_RESIZE which is returned by getch() when
>           the SIGWINCH handler has been called since the last call to
>           doupdate().

Yes. But it does not work well in gpm case, as shown in your previous letter.
When timed_wait is interrupted, fifo_push does not look at the winch flag.

> > > Possible solutions:
> > > 
> > > 1. Change gpm code to set SA_RESTART flag.
> > > 
> > >    Should be done, but this solves the problem only for ncurses+gpm, it
> > >    will pop up again for ncurses+<some other code out of ncurses' 
> > > control>. 
> > 
> > IMHO this is simplest solution without side effects.
> 
> I was specifically having SIGTSTP in mind; but if the change to gpm
> code also involved setting SA_RESTART for SIGWINCH, this would indeed
> have the side effect of 'breaking' the behavior you wish to see.
> 
> > > 2. Change ncurses code to prevent gpm code from installing its own SIGTSTP
> > >    handler; (temporarily) setting SIG_IGN at Gpm_Open() time would achieve
> > >    this.
> > > 
> > >    Again this is a workaround specific to gpm, some other code out of
> > >    ncurses' control can create the same kind of problem.
> > >    Also, the gpm SIGTSTP handler, besides creating a problem, does 
> > > something
> > >    useful; its function would have to be duplicated in ncurses code.
> > > 
> > > 3. Change ncurses code (back) to deal with interrupted read() system call 
> > > as
> > >    required.
> > > 
> > >    Obviously this is the preferred solution.
> > 
> > No, as it would break window resizing.
> 
> It shouldn't break it, just break immediate resizing if it is based on
> flawed assumptions.  And portable programs already have to deal with
> non-immediate resizing on some platforms where signals always restart.
> Programs written with ncurses in mind should start using KEY_RESIZE.

Right, but old programs can still install their own SIGWINCH and thus
disabling the KEY_RESIZE.


> Anyway, imo either the behavior of the library or the documentation
> definitely has to be changed.

Yes.

> The patch I sent doesn't go quite that far. It effectively only
> changes things if compiling with gmp support, in which case it
> is needed imo, given the current state of libgpm it is likely
> to be linked against, to prevent breaking lynx and (I assume)
> other programs.
> 
> Adding a global flag to control the behavior and a fucntion to set it
> would be better and should be straightforward; do you agree that
> that's a good approach?

It looks like temporary solution. Maybe adding xwgetch or some functions
to get ERR reason would be better.

   Alexander.