[PATCH] Cygwin: console: Improve the performance of peek_console

[Johannes Schindelin]

Hi Takashi,

On Mon, 14 Sep 2026, Takashi Yano wrote:

> On Sun, 13 Sep 2026 13:56:38 +0200 (CEST)
> Johannes Schindelin wrote:
> > 
> > On Fri, 4 Sep 2026, Takashi Yano wrote:
> > 
> > > Previously, peek_console() used PeekConsoleInput() to confirm
> > > whether the console input buffer has some input records. However,
> > > this need attaching to the console if the process does not attach to
> > > the console. To reduce that overhead, this patch uses
> > > WaitForSingleObject() with console input handle instead.
> > > WaitForSingleObject() works without attaching to the console, so
> > > this simplifies the peek_console() code.
> > 
> > Great! I like when Cygwin's code is taught to avoid expensive operations.
> > 
> > One issue, though:
> > 
> > > 
> > > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > > Reviewed-by:
> > > ---
> > >  winsup/cygwin/select.cc | 9 +--------
> > >  1 file changed, 1 insertion(+), 8 deletions(-)
> > > 
> > > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> > > index 592f6d14d..d565f4d5b 100644
> > > --- a/winsup/cygwin/select.cc
> > > +++ b/winsup/cygwin/select.cc
> > > @@ -1149,8 +1149,6 @@ peek_console (select_record *me, bool)
> > >        return 1;
> > >      }
> > >  
> > > -  INPUT_RECORD irec;
> > > -  DWORD events_read;
> > >    HANDLE h;
> > >    set_handle_or_return_if_not_open (h, me);
> > >  
> > > @@ -1161,12 +1159,7 @@ peek_console (select_record *me, bool)
> > >        else
> > >  	{
> > >  	  fh->acquire_input_mutex (mutex_timeout);
> > > -	  acquire_attach_mutex (mutex_timeout);
> > > -	  DWORD resume_pid = fh->attach_console (fh->get_owner ());
> > > -	  BOOL r = PeekConsoleInputW (h, &irec, 1, &events_read);
> > > -	  fh->detach_console (resume_pid, fh->get_owner ());
> > > -	  release_attach_mutex ();
> > > -	  if (!r || !events_read)
> > > +	  if (WaitForSingleObject (h, 0) != WAIT_OBJECT_0)
> > 
> > With this change, a host can report readiness without leaving any records
> > to process. Here is a concrete source counterexample in released
> > OpenConsole `v1.24.11911.0`:
> > 
> > 1. `SetConsoleActiveScreenBufferImpl()`
> >    (https://github.com/microsoft/terminal/blob/v1.24.11911.0/src/host/getset.cpp#L474-L498)
> >    calls `SetActiveScreenBuffer()`
> >    (https://github.com/microsoft/terminal/blob/v1.24.11911.0/src/host/output.cpp#L446-L476),
> >    which invokes `FlushAllButKeys()`.
> 
> I wonder why
>     if (_storage.empty())
>     {
>         ServiceLocator::LocateGlobals().hInputEvent.ResetEvent();
>     }
> is missing from:
> https://github.com/microsoft/terminal/blob/main/src/host/inputBuffer.cpp#L347-L353
> while flush()
> https://github.com/microsoft/terminal/blob/main/src/host/inputBuffer.cpp#L333-L337
> and read()
> https://github.com/microsoft/terminal/blob/main/src/host/inputBuffer.cpp#L466-L469
> reset the event.

This seems to be a genuine omission. I did research discussions, issues
and PRs in microsoft/terminal, but it does not seem to be discussed
directly. The closest I (or more precisely, GPT-6) found is:
https://github.com/microsoft/terminal/pull/14745#discussion_r1107879996
which is followed by an explanation that the `resetWaitEvent` mechanism
was replaced with `_storage.empty()`. So from my point of view, you're
right, `FlushAllButKeys()` should probably also reset the event if
`_storage` is empty. But take that with a big rock of salt, as I am
_definitely_ out of my depth when talking about `microsoft/terminal` code.

> > 2. If the queue contained only non-key records, it becomes empty while the
> >    input-available event remains signaled.
> > 3. An empty subsequent peek returns before the event-reset code
> >    (https://github.com/microsoft/terminal/blob/v1.24.11911.0/src/host/inputBuffer.cpp#L460-L470).
> >    With no other input activity repairing that state, Cygwin's new loop
> >    can repeatedly see "signaled handle" followed by "zero records."
> > 
> > In other words: changing active screen buffers can discard the remaining
> > non-key records without resetting the input-available event. The queue is
> > then empty but the handle remains signaled; an empty peek does not repair
> > the event. The new preliminary wait consequently succeeds repeatedly,
> > while `process_input_message(0)` finds zero records and returns
> > `input_processing` without changing readiness. `peek_console()` loops
> > without progress, so even a zero-timeout `select()` can hang. The old
> > preliminary peek explicitly broke out when its record count was zero.
> > 
> > This is not merely the normal distinction between a keypress and a
> > complete line. It is a stale host notification paired with an actually
> > empty queue. The source counterexample keeps Cygwin's worker paused
> > through Win32 input mode, with no new input or other reader repairing the
> > state.
> > 
> > I think we can keep the cheap `WaitForSingleObject()` check, but could we
> > let `process_input_message()` report an empty queue separately from
> > `input_processing`? If its `PeekConsoleInputW()` call finds no records,
> > `peek_console()` needs to stop retrying rather than spin on a handle that
> > stays signaled. Simply breaking on `input_processing` would also stop when
> > we consumed input but hadn't yet completed a canonical line, so we need to
> > keep those cases separate. Please handle input errors explicitly, too.
> 
> What about calling read() with AmountToRead == 0
> https://github.com/microsoft/terminal/blob/main/src/host/inputBuffer.cpp#L372-L475
> ?

Unfortunately, there's an early return in that function if `OutEvents` is
empty, _before_ the event is reset:
https://github.com/microsoft/terminal/blob/dcfbcdd8639813994734ced60abec37474cb3ec7/src/host/inputBuffer.cpp#L462-L469

Ciao,
Johannes

> 
> It seems that read() with length 0 resets the event if the
> input buffer is empty.
> 
> What do you think?
> 
> -- 
> Takashi Yano <takashi.yano@nifty.ne.jp>
>