fix(windows): closing panes sometimes freezes wezterm #5977
Conversation
|
@RasmusN could you give a try to wezterm in this PR and see if helps with the problem where closing panes hang, I will be testing this over the coming week. |
pty/src/win/pseudocon.rs
Outdated
| // read up to 1000 bytes | ||
| while let Ok(num_byes_read) = self.output.read(&mut buffer[..]) { | ||
| if num_byes_read == 0 { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This won't work, because until recently you must read from the output pipe concurrently while ClosePseudoConsole is called. This is also true for the version of ConPTY that wezterm uses.
However, starting microsoft/terminal#17704 you can do something else: You can first call the new ClosePseudoConsole and then (on the same thread; not concurrently) read from the output pipe until it is closed on you (returns an error). Basically, you can do what you wrote, but with the order flipped.
If you don't want to upgrade ConPTY now, you can do something like this as a temporary workaround (pseudo-code; I keep forgetting what valid Rust syntax looks like lol):
fn drop(&mut self) {
// A `HPCON` is a struct consisting of 3 HANDLEs:
// A pipe for communicating with the PTY, a handle to keep it alive
// until ClosePseudoConsole is called, and a process handle to the
// underlying conhost process.
unsafe {
let handles = self.con as *mut [HANDLE; 3];
for i in ..3 {
CloseHandle(handles[i]);
handles[i] = std::ptr::null();
}
}
// read up to 1000 bytes
while let Ok(num_byes_read) = self.output.read(&mut buffer[..]) {
if num_byes_read == 0 {
break;
}
}
// This won't do anything but deallocate the handle.
unsafe { (CONPTY.ClosePseudoConsole)(self.con) };
}That aside, ReleasePseudoConsole is a new function which does the same as the unsafe code above but only for the 2nd handle (which keeps conhost alive). I've started writing documentation for it here: MicrosoftDocs/Console-Docs#323
Here's a preview of the documentation: https://github.com/MicrosoftDocs/Console-Docs/blob/dev/lhecker/24h2/docs/releasepseudoconsole.md
Ideally Wezterm should call ReleasePseudoConsole (or close the 2nd HPCON handle) right after attaching an application to ConPTY, about here: https://github.com/FrancescElies/wezterm/blob/a25e7be9bc6c68290ed465b6c1aa780cfee9b793/pty/src/win/pseudocon.rs#L189-L191
It should then stop returning a process handle from that function and not use it to track the shell/application lifetime anymore. Instead, it should read from the output pipe until the pipe has been closed on it by the PTY. If wezterm wants to kill all attached console applications prematurely (e.g. when closing the tab), it can call the upcoming non-blocking ClosePseudoConsole, or it can close all 3 handles like in the example above.
Long term, wezterm should use the proper ConPTY APIs of course, but short term it would be absolutely fine to cast HPCON to an array of handles. Its struct layout is part of the kernel ABI and so it won't change in any existing version and is unlikely to change any time soon.
There was a problem hiding this comment.
will read your message in detail and change this pr accordingly on monday when I am back in a windows box
There was a problem hiding this comment.
This won't work, because until recently you must read from the output pipe concurrently while
ClosePseudoConsoleis called. This is also true for the version of ConPTY that wezterm uses.
Understood, I was suspecting that, in the docs I read that is recommended to drain
communication channels in a separate thread, but I thought maybe I could get
away without creating a separate thread and keep things simpler.
I couldn't put that block after ClosePseudoConsole because that's the
function that was blocking but you solved that problem for me below.
However, starting microsoft/terminal#17704 you can do something else: You can first call the new
ClosePseudoConsoleand then (on the same thread; not concurrently) read from the output pipe until it is closed on you (returns an error). Basically, you can do what you wrote, but with the order flipped.
One part of me is eager to upgrade and learn about conpty and wezterm but given
that I don't know wezterm's codebase and only know about conpty what you taught
me I will leave that for a separate pr and keep this pr with the minimal patch
that solves the closing panes hanging problem.
If you don't want to upgrade ConPTY now, you can do something like this as a temporary workaround (pseudo-code; I keep forgetting what valid Rust syntax looks like lol):
fn drop(&mut self) { // A `HPCON` is a struct consisting of 3 HANDLEs: // A pipe for communicating with the PTY, a handle to keep it alive // until ClosePseudoConsole is called, and a process handle to the // underlying conhost process. unsafe { let handles = self.con as *mut [HANDLE; 3]; for i in ..3 { CloseHandle(handles[i]); handles[i] = std::ptr::null(); } } // read up to 1000 bytes while let Ok(num_byes_read) = self.output.read(&mut buffer[..]) { if num_byes_read == 0 { break; } } // This won't do anything but deallocate the handle. unsafe { (CONPTY.ClosePseudoConsole)(self.con) }; }
Thanks for the example, for this PR this will be the easiest I will just copy pasta your code and make it compile.
That aside,
ReleasePseudoConsoleis a new function which does the same as the unsafe code above but only for the 2nd handle (which keeps conhost alive). I've started writing documentation for it here: MicrosoftDocs/Console-Docs#323 Here's a preview of the documentation: https://github.com/MicrosoftDocs/Console-Docs/blob/dev/lhecker/24h2/docs/releasepseudoconsole.mdIdeally Wezterm should call
ReleasePseudoConsole(or close the 2nd HPCON handle) right after attaching an application to ConPTY, about here: https://github.com/FrancescElies/wezterm/blob/a25e7be9bc6c68290ed465b6c1aa780cfee9b793/pty/src/win/pseudocon.rs#L189-L191It should then stop returning a process handle from that function and not use it to track the shell/application lifetime anymore. Instead, it should read from the output pipe until the pipe has been closed on it by the PTY. If wezterm wants to kill all attached console applications prematurely (e.g. when closing the tab), it can call the upcoming non-blocking
ClosePseudoConsole, or it can close all 3 handles like in the example above.Long term, wezterm should use the proper ConPTY APIs of course, but short term it would be absolutely fine to cast
HPCONto an array of handles. Its struct layout is part of the kernel ABI and so it won't change in any existing version and is unlikely to change any time soon.
Even though your explanations are very clear I will leave that for a separate
PR, I am sure if I attempt this I will misinterpret things and will need you to
hold my hand in the process.
I will try to maximize the chances of just fixing my issue first and later on
attempt that or at least create an issue linking your comment in case someone
else feels like trying the upgrade.
FrancescElies
force-pushed
the
cesc/fix-hang-on-close-pane
branch
from
a25e7be
to
68fd304
Compare
|
@RasmusN pr updated |
|
@RasmusN did you have any issues with this build, so far using this pr today wezterm didn't hang for me anymore. |
FrancescElies
changed the title
FrancescElies
changed the title
I wasn't able to build it on my computer. I'm getting the following error: Which has most likely nothing to do with your changes. However, I'll dig deeper into it tonight, looks like some perl library missing. |
|
@RasmusN getting wezterm to build was a pain for me because of perl, but that issue should be gone with the latest release, did you install perl? Could you share a bit more of your error, that line doesn’t say much, you can share really long traces with a collapsible markdown block :) |
Sure! output$ cargo build --release Caused by: --- stderr Error configuring OpenSSL build: note: run with edit: Managed to solve the issue above by installing strawberryperl... however I got stuck on this:
$ cargo build --release
Compiling openssl-sys v0.9.103
Compiling libz-sys v1.1.18
Compiling image v0.25.2
Compiling terminfo v0.9.0
Compiling mlua-sys v0.6.2
Compiling wezterm-input-types v0.1.0 (C:\Users\RasmusNordström\projects\wezterm\wezterm-input-types)
Compiling fancy-regex v0.11.0
Compiling wezterm-bidi v0.2.3 (C:\Users\RasmusNordström\projects\wezterm\bidi)
Compiling vtparse v0.6.2 (C:\Users\RasmusNordström\projects\wezterm\vtparse)
Compiling same-file v1.0.6
Compiling serial-core v0.4.0
Compiling libssh2-sys v0.3.0
Compiling libssh-rs-sys v0.2.4
Compiling instant v0.1.13
Compiling fixedbitset v0.4.2
Compiling openssl v0.10.66
Compiling hex v0.4.3
Compiling memmem v0.1.1
Compiling libm v0.2.8
Compiling foreign-types-shared v0.1.1
error: failed to run custom build command for openssl-sys v0.9.103
Caused by: --- stderr thread 'main' panicked at C:\Users\RasmusNordström.cargo\registry\src\index.crates.io-6f17d22bba15001f\openssl-src-300.3.1+3.3.1\src\lib.rs:621:9: Error configuring OpenSSL build: note: run with RUST_BACKTRACE=1 environment variable to display a backtrace It seems to be an unicode issue (like its 1998) with the path name |
|
I was able to build it and have not experienced another hang. Before it would hang pretty consistently when the panes had some processes running. Thanks! |
@RasmusN being on a non US/UK setup is never fun, lately I faced StrawberryPerl/Perl-Dist-Strawberry#135 which is now solved, that kept me from building wezterm for a while. Colleagues with special characters faced also this one fastapi/typer#277. Now again If you can rename your user and feel fine make an aberration with your last name, just rename your username If you can't, try at least to move your projects folder to another location In short, if it's not to risky to rename your user, do that; that will save you trouble for the future. |
Thank mostly @lhecker for this one, he basically spotted where the problem was and told me exactly what to do about it. |
|
👋 @wez & @Cammisuli I hope you don't find my @ to you impolite, probably you are quite busy or this PR might have gone unnoticed under your radar. PR summaryWhen closing wezterm panes on windows it's quite common to see wezterm hanging indefinitely. After this pr wezterm will not freeze when closing panes on windows, the solution was proposed by @lhecker, one of the main contributors to https://github.com/microsoft/terminal where conpty code lives. @clemenscodes and me can confirm that we don't see wezterm freeze anymore. Given the above I think this PR is ready for you to have a look when you have time. |
Sorry for late reply. I'm using a computer from work so I'm not able to change username. I was able to move it there with an admin account but then I realized I would have to reinstall cargo, VS C++ dev kit, etc on the admin account... Anyway, great job with both finding the bug and providing a solution @FrancescElies! Hopefully this will be merged as soon as possible. I'm starting to get a bit tired of submitting a tiny prayer before closing a pane/tab. |
|
@RasmusN we use windows at work too.
I feel your pain, if it wouldn't be because there is no viable alternative to wezterm, something with tmux like capabilities that works crossplatform across the the big three windows, linux & macos I would have probably moved on to something else. In the meanwhile until I ge approval to run on ci this pr you can download it from ci in my fork, see here at the bottom of the page you have an installer, could you give it a try? |
Awesome, thanks! Will try it out and get back to you. |
|
I've used it the whole day, not a single freeze so far. |
pty/src/win/conpty.rs
Outdated
| @@ -1,5 +1,5 @@ | |||
| use crate::cmdbuilder::CommandBuilder; | |||
| use crate::win::psuedocon::PsuedoCon; | |||
| use crate::win::pseudocon::PsuedoCon; | |||
There was a problem hiding this comment.
Randomly passing by, but shouldn't you rename the PsuedoCon struct as well while you are at it ?
There was a problem hiding this comment.
thanks, missed that one, addressed with the latest update
Wezterm hangs randomly when closing panes on windows. See microsoft/terminal#17716
FrancescElies
force-pushed
the
cesc/fix-hang-on-close-pane
branch
from
68fd304
to
3b68891
Compare
|
Thanks for this! While I'm not opposed to some low level poking, if this is fixed with an updated conpty.dll, I think I'd rather just update that dll and avoid this complexity, and also avoid having to later remember to resolve this properly. Last time conpty was updated was in e7fe7c0 using pre-built binaries that @DHowett published. I'd like to grab a newer version of those and update, but I'm not sure how best to locate the current version if any has been updated since we conducted the initial experiment with this! |
|
I understand, i would gladly try to upgrade conpty myself but I won’t be able to do that until April next year. |
Based on #5977
Problem
On windows closing panes sometimes hangs wezterm.
These problem is related with how
conpty.dllis being used this has been discussed in microsoft/terminal#17716The problem seems to be mentioned in the docs at creating-a-pseudoconsole-session#ending-the-pseudoconsole-session
This pr
Hopefully will close #5882, could it help with related issues too #5496 #5432?
This pr manually closes handles before closing pseudoconsole to avoid that call from blocking.
Tasks
conpty.dllNot strictly necessary to solve this issue in.
Here leonard added instructions with the necessary steps to do that upgrade.