Re: Another pipe-related problem?

[Ken Brown <kbrown@cornell.edu>]

On 11/9/2021 5:20 PM, Ken Brown via Cygwin wrote:
> On 11/9/2021 5:16 PM, Ken Brown via Cygwin wrote:
>> On 11/9/2021 9:11 AM, Ken Brown via Cygwin wrote:
>>> On 11/9/2021 5:55 AM, Henry S. Thompson wrote:
>>>> As you may know, the XEmacs situation is complicated.  The old source
>>>> repo (bitbucket.org/xemacs) no longer exists.  There's a fork that's
>>>> still being maintained, but it's not widely publicised.  That's the
>>>> one I'm working with -- are you aware of this.
>>>
>>> I was aware that the bitbucket repo didn't exist, because I tried to get the 
>>> sources there.  But I didn't know about the fork.  Please point me to it, or 
>>> just make a tarball available to me somehow.
>>>
>>>> Here are the immediate contexts from the sources for the xemacs
>>>> sources in the above backtrace, might be enough to check your
>>>> hypothesis:
>>>>
>>>> sysdep.c:
>>>>
>>>>    retry_read_1 (int fildes, void *buf, size_t nbyte, int allow_quit)
>>>>    {
>>>>      ssize_t rtnval;
>>>>
>>>>      while ((rtnval = read (fildes, buf, nbyte)) == -1
>>>>             && (errno == EINTR))                         <<<<<<<<<<<<<<<<<<<<
>>>>        {
>>>>          if (allow_quit)
>>>>            QUIT;
>>>>        }
>>>>      return rtnval;
>>>>    }
>>> I'll have to reproduce the hang myself in order to test this (or maybe you 
>>> could test it), but I now have a new guess: If the read call above keeps 
>>> failing with EINTR, then we're in an infinite loop.  This could happen 
>>> because of the following code in fhandler_pipe::raw_read:
>>>
>>>    DWORD waitret = cygwait (read_mtx, timeout);
>>>    switch (waitret)
>>>      {
>>>      case WAIT_OBJECT_0:
>>>        break;
>>>      case WAIT_TIMEOUT:
>>>        set_errno (EAGAIN);
>>>        len = (size_t) -1;
>>>        return;
>>>      default:
>>>        set_errno (EINTR);
>>>        len = (size_t) -1;
>>>        return;
>>>      }
>>>
>>> Takashi, is EINTR really the appropriate errno in the default case?  Isn't 
>>> cygwait supposed to handle signals?
>>
>> I was able to build XEmacs and reproduce the problem.  My guess was wrong, 
>> though my question to Takashi still stands.  I think the infinite loop is 
>> actually caused by a bug in fhandler_pipe::raw_read that only affects 
>> non-blocking pipes (which is what we have in XEmacs).
>>
>> Consider the following code in fhandler_pipe::raw_read:
>>
>>        status = NtReadFile (get_handle (), evt, NULL, NULL, &io, ptr,
>>                 len1, NULL, NULL);
>>        if (evt && status == STATUS_PENDING)      <<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>      {
>>        waitret = cygwait (evt, INFINITE, cw_cancel | cw_sig);
>> [...]
>>      }
>>
>> In the non-blocking case, evt == NULL, but we still might have status == 
>> STATUS_PENDING.  We then should wait on get_handle() to let NtReadFile finish. 
>> By not waiting, we end up using a garbage value from io.Information, leading 
>> to an infinite loop in drain_signal_event_pipe.

Nope, that doesn't seem to be the issue.  Even after fixing this, I still see an 
infinite loop.  Probably NtReadFile finishes quickly enough that io.Information 
is in fact valid by the time we test it.  Back to the drawing board.

> BTW, a quick glance at raw_write suggests that there might be a similar bug 
> there, but I'll have to look more closely.

Ken