* [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check. @ 2024-02-02 16:18 Takashi Yano 2024-02-03 14:53 ` Johannes Schindelin 0 siblings, 1 reply; 4+ messages in thread From: Takashi Yano @ 2024-02-02 16:18 UTC (permalink / raw) To: cygwin-patches; +Cc: Takashi Yano If disable_master_thread flag is set between the code checking that flag not be set and the code acquiring input_mutex, input record is processed once after setting disable_master_thread flag. This patch prevents that. Fixes: d4aacd50e6cf ("Cygwin: console: Add missing input_mutex guard.") Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> --- winsup/cygwin/fhandler/console.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc index 6a42b4949..1c8d383cd 100644 --- a/winsup/cygwin/fhandler/console.cc +++ b/winsup/cygwin/fhandler/console.cc @@ -420,6 +420,12 @@ fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp) } WaitForSingleObject (p->input_mutex, mutex_timeout); + /* Ensure accessing input recored is not disabled. */ + if (con.disable_master_thread) + { + ReleaseMutex (p->input_mutex); + continue; + } total_read = 0; switch (cygwait (p->input_handle, (DWORD) 0)) { @@ -4545,8 +4551,6 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons) return; } const _minor_t unit = cons->get_minor (); - if (con.disable_master_thread == x) - return; cons->acquire_input_mutex (mutex_timeout); con.disable_master_thread = x; cons->release_input_mutex (); -- 2.43.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check. 2024-02-02 16:18 [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check Takashi Yano @ 2024-02-03 14:53 ` Johannes Schindelin 2024-02-03 15:47 ` Takashi Yano 2024-02-03 18:48 ` Jeremy Drake 0 siblings, 2 replies; 4+ messages in thread From: Johannes Schindelin @ 2024-02-03 14:53 UTC (permalink / raw) To: Takashi Yano; +Cc: cygwin-patches Hi Takashi, On Sat, 3 Feb 2024, Takashi Yano wrote: > If disable_master_thread flag is set between the code checking that > flag not be set and the code acquiring input_mutex, input record is > processed once after setting disable_master_thread flag. This patch > prevents that. > > Fixes: d4aacd50e6cf ("Cygwin: console: Add missing input_mutex guard.") > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> Thank you for double-checking this (and for finding and fixing the bug). I wonder what could be a symptom of this bug. I ask because we have noticed a couple of inexplicable hangs in GitHub workflow runs in the Git for Windows and the MSYS2 projects, hangs that are almost certainly due to the ConPTY code in Cygwin, and I hope to figure out what causes them, and even more importantly, how to fix those. Maybe the bug you fixed in this patch could explain this? Concretely, the hangs occur typically when some `pacman` process (a package manager using the MSYS2 runtime, i.e. the Cygwin runtime with several dozen patches on top) calls a few non-Cygwin processes. Those processes seem to succeed, but there is an extra `pacman` process left hanging around (reported using the same command-line as its parent process, indicating a `fork()`ed child process or maybe that signal-handler that is spawned for every non-Cygwin child process) and at that point the program hangs indefinitely (or at least until the GitHub workflow run times out after 6 hours). I was not able to obtain any helpful stacktraces, they all seem to make no sense, I only vaguely remember that one thread was waiting for an object, but that could be a false flag. Stopping those hanging `pacman` processes via `wmic process ... delete` counter-intuitively fails to result in `pacman` to exit with a non-zero exit code. Instead, the program now runs to completion successfully! Most recently, these hangs became almost reliably reproducible, when we started changing a certain GitHub workflow that runs on a Windows/ARM64 build agent. I suspect that Windows/ARM64 just makes this much more likely, but that the bug is also there on regular Windows/x86_64. What changed in the GitHub workflow is a new PowerShell script that runs `pacman` a couple of times, trying to update packages, and after that force-reinstalls a certain package for the benefit of its post-install script. This post-install script is run using the (MSYS) Bash, and it calls among other things (non-MSYS) `git.exe`. And that's where it hangs almost every time. When I log into the build agent via RDP, I do not see any `git.exe` process running, but the same type of hanging `pacman` process as indicated above. Using the `wmic` command to stop the hanging process lets the GitHub workflow continue without any indication of failure. Running the PowerShell script manually succeeds every single time, so I think the hang might be connected to running things headlessly. Do you have any idea what the bug could be? Or how I could diagnose this better? Attaching via `gdb` only produces unhelpful stacktraces (that may even be bogus, by the looks of it). Or do you think that your patch that I am replying to could potentially fix this problem? How could the code be improved to avoid those hangs altogether, or at least to make them easier to diagnose? Ciao, Johannes > --- > winsup/cygwin/fhandler/console.cc | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc > index 6a42b4949..1c8d383cd 100644 > --- a/winsup/cygwin/fhandler/console.cc > +++ b/winsup/cygwin/fhandler/console.cc > @@ -420,6 +420,12 @@ fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp) > } > > WaitForSingleObject (p->input_mutex, mutex_timeout); > + /* Ensure accessing input recored is not disabled. */ > + if (con.disable_master_thread) > + { > + ReleaseMutex (p->input_mutex); > + continue; > + } > total_read = 0; > switch (cygwait (p->input_handle, (DWORD) 0)) > { > @@ -4545,8 +4551,6 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons) > return; > } > const _minor_t unit = cons->get_minor (); > - if (con.disable_master_thread == x) > - return; > cons->acquire_input_mutex (mutex_timeout); > con.disable_master_thread = x; > cons->release_input_mutex (); > -- > 2.43.0 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check. 2024-02-03 14:53 ` Johannes Schindelin @ 2024-02-03 15:47 ` Takashi Yano 2024-02-03 18:48 ` Jeremy Drake 1 sibling, 0 replies; 4+ messages in thread From: Takashi Yano @ 2024-02-03 15:47 UTC (permalink / raw) To: cygwin-patches On Sat, 3 Feb 2024 15:53:29 +0100 (CET) Johannes Schindelin wrote: > I wonder what could be a symptom of this bug. I ask because we have > noticed a couple of inexplicable hangs in GitHub workflow runs in the Git > for Windows and the MSYS2 projects, hangs that are almost certainly due to > the ConPTY code in Cygwin, and I hope to figure out what causes them, and > even more importantly, how to fix those. Maybe the bug you fixed in this > patch could explain this? I don't think so. The symptom of this bug is that console input for non-cygwin process can be lost or reordered at the starting timing of that program for about 40 msec. > Concretely, the hangs occur typically when some `pacman` process (a > package manager using the MSYS2 runtime, i.e. the Cygwin runtime with > several dozen patches on top) calls a few non-Cygwin processes. Those > processes seem to succeed, but there is an extra `pacman` process left > hanging around (reported using the same command-line as its parent > process, indicating a `fork()`ed child process or maybe that > signal-handler that is spawned for every non-Cygwin child process) and at > that point the program hangs indefinitely (or at least until the GitHub > workflow run times out after 6 hours). > > I was not able to obtain any helpful stacktraces, they all seem to make no > sense, I only vaguely remember that one thread was waiting for an object, > but that could be a false flag. > > Stopping those hanging `pacman` processes via `wmic process ... delete` > counter-intuitively fails to result in `pacman` to exit with a non-zero > exit code. Instead, the program now runs to completion successfully! > > Most recently, these hangs became almost reliably reproducible, when we > started changing a certain GitHub workflow that runs on a Windows/ARM64 > build agent. I suspect that Windows/ARM64 just makes this much more > likely, but that the bug is also there on regular Windows/x86_64. > > What changed in the GitHub workflow is a new PowerShell script that runs > `pacman` a couple of times, trying to update packages, and after that > force-reinstalls a certain package for the benefit of its post-install > script. This post-install script is run using the (MSYS) Bash, and it > calls among other things (non-MSYS) `git.exe`. And that's where it hangs > almost every time. > > When I log into the build agent via RDP, I do not see any `git.exe` > process running, but the same type of hanging `pacman` process as > indicated above. Using the `wmic` command to stop the hanging process lets > the GitHub workflow continue without any indication of failure. > > Running the PowerShell script manually succeeds every single time, so I > think the hang might be connected to running things headlessly. > > Do you have any idea what the bug could be? Or how I could diagnose this > better? Attaching via `gdb` only produces unhelpful stacktraces (that may > even be bogus, by the looks of it). Or do you think that your patch that I > am replying to could potentially fix this problem? How could the code be > improved to avoid those hangs altogether, or at least to make them easier > to diagnose? Thanks for details of your problem. Unfortunately, I have currently no idea. Non-cygwin process is now treated very different from cygwin process because it of course does not use cygwin functionality. For example: (1) Cygwin setups pseudo-console for non-cygwin process in pty. Because pty is just a pipe for non-cygwin program and some programs do not work correctly on the pipe. (2) Cygwin restores the default console mode for non-cygwin process if it is running in console. This is because, cygwin uses xterm compatible mode for cygwin process which is enabled by ENABLE_VIRTUAL_TYERMINAL_PROCESSING and ENABLE_VIRTUAL_TERMINAL_INPUT. However, less cygwin users runs non-cygwin process in cygwin than MSYS2 users. MSYS2 users uses non-MSYS2 program under MSYS2 environment on a daily basis, so probability to hit a bug of (1) and (2) is relatively higher than cygwin users. If you attached gdb to non-msys2 program and failed to retrieve meaningfull information, how about attaching gdb to the stub process? Note that the stub process is hidden from normal cygwin tools such as 'ps'. However, it is real parent process of the non-msys2 program and you can find it in taskmanager. -- Takashi Yano <takashi.yano@nifty.ne.jp> ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check. 2024-02-03 14:53 ` Johannes Schindelin 2024-02-03 15:47 ` Takashi Yano @ 2024-02-03 18:48 ` Jeremy Drake 1 sibling, 0 replies; 4+ messages in thread From: Jeremy Drake @ 2024-02-03 18:48 UTC (permalink / raw) To: cygwin-patches Thanks for taking the time to write this up, this issue has been bugging me for years... (see also: https://cygwin.com/pipermail/cygwin-patches/2021q4/011638.html) On Sat, 3 Feb 2024, Johannes Schindelin wrote: > Concretely, the hangs occur typically when some `pacman` process (a > package manager using the MSYS2 runtime, i.e. the Cygwin runtime with > several dozen patches on top) calls a few non-Cygwin processes. Those > processes seem to succeed, but there is an extra `pacman` process left > hanging around (reported using the same command-line as its parent > process, indicating a `fork()`ed child process or maybe that > signal-handler that is spawned for every non-Cygwin child process) and at > that point the program hangs indefinitely (or at least until the GitHub > workflow run times out after 6 hours). I don't think this requires running non-Cygwin children - I see this most often when pacman is using GPGME to validate signatueres. That fork/exec's (Cygwin) gpg. > I was not able to obtain any helpful stacktraces, they all seem to make no > sense, I only vaguely remember that one thread was waiting for an object, > but that could be a false flag. My recollection when I tried to debug was that every debugger I tried got an error trying to get the context of the main thread of the hung process. There was another thread, which seemed to be for Cygwin signal handling, which was blithely waiting for an object, but I kind of expect that's what it was supposed to be doing. > Stopping those hanging `pacman` processes via `wmic process ... delete` > counter-intuitively fails to result in `pacman` to exit with a non-zero > exit code. Instead, the program now runs to completion successfully! > Do you have any idea what the bug could be? Or how I could diagnose this > better? Attaching via `gdb` only produces unhelpful stacktraces (that may > even be bogus, by the looks of it). Or do you think that your patch that I > am replying to could potentially fix this problem? How could the code be > improved to avoid those hangs altogether, or at least to make them easier > to diagnose? ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-03 18:48 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-02 16:18 [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check Takashi Yano 2024-02-03 14:53 ` Johannes Schindelin 2024-02-03 15:47 ` Takashi Yano 2024-02-03 18:48 ` Jeremy Drake
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).