On Fri, 9 May 2025 22:07:57 +0900
Takashi Yano wrote:
> On Thu, 8 May 2025 09:19:47 +0200 (CEST)
> Johannes Schindelin wrote:
> > Hi Takashi,
> >
> > On Wed, 7 May 2025, Takashi Yano wrote:
> >
> > > ... and restore it when app exits. The commit 0bfd91d57863 has a bug
> > > that the console mode is stored into the shared memory when both:
> > > (1) cygwin process is started from non-cygwin process.
> > > (2) cygwin process started from non-cygwin process exits.
> > > (1) is intended, but (2) is not. Due to (2), the stored console mode
> > > is unexpectedly broken when the cygwin process exits. Then the mode
> > > restored will be not as expected. This causes undesired console mode
> > > in the use case that cygwin and non-cygwin apps are mixed.
> > >
> > > With this patch, the console mode will stored only in the case (1).
> > > This is done by putting the code, which stores the console mode, into
> > > fhandler_console::open() rather than fhandler_console::set_input_mode()
> > > and fhandler_console::set_output_mode().
> > >
> > > Fixes: 0bfd91d57863 ("Cygwin: console: tty::restore really restores the
> > > previous mode")
> > > Reported-by: Johannes Schindelin <[email protected]>
> > > Signed-off-by: Takashi Yano <[email protected]>
> >
> > Thank you! I can confirm that this re-fixes the problem fixed in
> > 3312f2d21f (Cygwin: console: Redesign mode set strategy on close().,
> > 2025-03-03), which was in response to a report I received in
> > https://github.com/microsoft/git/issues/730.
> >
> > I also asked the reporter of the bug you fixed in 114cbda779 (Cygwin:
> > console: tty::restore really restores the previous mode, 2025-03-21)
> > (which was the cause for the regression I reported) to verify that _their_
> > bug did not reappear, see
> > https://github.com/msys2/msys2-runtime/issues/268#issuecomment-2858071841
> >
> > Finally, I spent quite a few hours to write an automated test that
> > imitates my reproducer, and integrated it into Git for Windows' CI builds:
> > https://github.com/git-for-windows/msys2-runtime/pull/95/commits/7acbb031654404c9fd711eee9974c88475de98fd
>
> Thanks for testing.
>
> > However, I still have concerns about the patch. It still lets the Cygwin
> > runtime operate under the assumption that it can control the console, even
> > though other process can interact with the same console in _overlapping_
> > lifecycles.
> >
> > Keep in mind that the bug I reported twice involves a Cygwin process that
> > spawns a non-Cygwin process which writes to the console _after_ the Cygwin
> > process exited (and restored the console mode).
> >
> > It looks plausible to me that this storing, setting, then re-setting of
> > the console mode by the Cygwin runtime will never cease to be fragile and
> > will always be prone to surface bugs that are similar, bug not identical
> > to the two bugs referenced above.
> >
> > Therefore I fail to convince myself that the current design is okay, and
> > expect that, in the long run, the Cygwin runtime will stop to toggle the
> > console modes as if the console were not serving other, non-Cygwin
> > processes, too.
> >
> > I do see that you changed some code so that it is only run when Cygwin is
> > the console owner. That triggers the following questions, answers to which
> > I could not find in the commit message (and neither in the diff):
> >
> > - Now that the mode is left alone when the console is owned by a
> > non-Cygwin process, which code is broken? There must have been a reason
> > to change the console mode in the first place, after all.
>
> What situation do you assume? I think I designed so that the console mode
> is restored to tty::cygwin when non-cygwin process exits.
^^^^^^^^^^^^^^^^^^^^^^^^^
I meant "when the last non-cygwin process exits"
--
Takashi Yano <[email protected]>
