fix: Handle signals before crossterm events #6170
Conversation
|
Good catch ! |
Agreed, but I'm embarrassed to say I'm not quite sure. In any case I think it makes intuitive sense to handle the signals (kind of like a higher priority "interrupt") before the input. If anyone has any suggestions for a comment to add, I'll gladly add it! |
Something like this: |
|
Right, the biased part isn't confusing. It's confusing that checking the event stream future changes the behavior of |
|
This is with my patch This is without the patch: For me, the keyboard protocol is also correctly identified with the signals processed first. What's the issue you're seeing @the-mikedavis ? |
There was a problem hiding this comment.
Ok it looks like we're locking up crossterm's event polling system by trying to set up multiple concurrent readers. There's a mutex that allows only one reader to read/poll for events at a time. The input_stream.next() locks that mutex and then crossterm::terminal::supports_keyboard_enhancement tries to lock the mutex again to read the response to the query. The input event stream already holds the lock though so this times out unless a keyboard event arrives before the timeout. This has an effect you can test manually: C-z will hang the editor for two seconds on master but if you hit a button on your keyboard faster than the 2s, Helix will suspend properly. That's because the event stream future receives a KeyEvent and can drop its hold on the mutex. Then the keyboard enhancement protocol query can acquire the mutex and read the response which was pending that whole time.
I will open up an issue in the crossterm repo to talk about refactoring queries against the host terminal. This isn't just limited to supports_keyboard_enhancement: I believe it should also be possible to lock ourselves up with the cursor position query. We want other query capabilities in crossterm anyways like checking for synchronized output support and querying the shape of the cursor and title of the terminal on startup, so this is something I've been meaning to discuss anyways.
In the meantime I will merge this as a workaround since it's very easy to trigger #6149 and I don't see any downsides to handling signals first.
the-mikedavis
changed the title
fg no longer wait for 2s timeout
|
Opened up crossterm-rs/crossterm#763 |
This is a workaround for a freeze when suspending Helix with C-z on non-Windows systems. The check for the keyboard enhancement protocol locks up crossterm's internal event reading/polling system by trying to set up multiple concurrent readers. `input_stream.next()` sets up one reader looking for regular crossterm events while the `supports_keyboard_enhancement` query sets up another looking for internal events. The latter hangs for two seconds or until the former yields an event. By handling signals first we don't lock up the mutex by trying to read keyboard events.
Since 79bf5e3
C-zandfghas taken 2 seconds for me. This small change fixes that.