Re: [PATCH v2] Cygwin: console: re-enable the master thread before selecting cygwin input mode

On Sat, 4 Jul 2026 14:38:55 +0200 (CEST)
Johannes Schindelinwrote:
> Hi Takashi,
> 
> On Tue, 30 Jun 2026, Takashi Yano wrote:
> 
> > From: Johannes Schindelin <[email protected]>
> > 
> > When a cygwin program and a non-cygwin program run in the same foreground
> > process group (for example the pipeline `cat | ping`), Ctrl-C stopped
> > interrupting the cygwin program after "Cygwin: console: Ensure the master
> > thread runs only when it is supposed to".
> > 
> > The console only delivers Ctrl-C as a raw 0x03 byte (which the console
> > master thread reads and turns into a SIGINT for the foreground process
> > group) while that thread is live. When it is suspended or disabled,
> > set_input_mode (tty::cygwin) instead requests ENABLE_PROCESSED_INPUT, so
> > the console raises a CTRL_C_EVENT and the 0x03 byte never reaches the
> > master thread. The referenced commit reordered the two enable paths,
> > bg_check () and post_open_setup (), so that set_input_mode (tty::cygwin)
> > runs while disable_master_thread is still set; that leaves
> > ENABLE_PROCESSED_INPUT on and the cygwin program never receives its SIGINT.
> > 
> > Clear disable_master_thread before selecting cygwin input mode in those two
> > paths, so the mode is configured with the master thread already live and
> > ENABLE_PROCESSED_INPUT stays off. The disable paths and the synchronous
> > suspension that the referenced commit added are left unchanged, so
> > non-cygwin programs still get the master thread reliably suspended.
> > 
> > Fixes: 733d5a953fa9 ("Cygwin: console: Ensure the master thread runs only when it is supposed to")
> > Assisted-by: Opus 4.8
> > Signed-off-by: Johannes Schindelin <[email protected]>
> > Co-Authored-by: Takashi Yano <[email protected]>
> > Reviewed-by: Takashi Yano <[email protected]>
> > ---
> 
> Thank you for extending this patch! My only concern: The commit message
> still says "the two enable paths, bg_check () and post_open_setup ()" and
> "Clear disable_master_thread before selecting cygwin input mode in those
> two paths."
> 
> With the `cleanup_for_non_cygwin_app()` hunk added, this needs to say
> three paths and enumerate them. Also, "clear" isn't accurate for the
> cleanup path where the argument is `con.owner == GetCurrentProcessId()`,
> not literal `false`. Maybe "set `disable_master_thread` to its target
> value before..."?

This is for a special case, that itself is a master process which calls
exec() for a non-cygwin app. In this case, subsequent set_input_mode()
call sets the mode to tty::restore, and the master thread should be kept
'disabled' until the process exits.

> The functional change looks like a real improvement over the version I had
> sent. The reorder is internally consistent, the asymmetry with the
> "disable" paths is correct, and the change is a strict improvement (no
> regressions for the owner of the `tty:restore` sub-cases, and closes a
> latent bug for the non-owner `tty::cygwin` sub-case). I integrated it into
> https://github.com/git-for-windows/msys2-runtime/pull/131 just to be extra
> certain, and the AutoHotKey-based UI tests still show that the tested
> scenarios do not regress.

I modified the commit message regarding above commnet, and pushed master
and cygwin-3_6-branch.

Thank you very much!

-- 
Takashi Yano <[email protected]>