Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch for RPC handler race condition causing a crash #2940

Merged
merged 1 commit into from
May 10, 2018

Conversation

umpc
Copy link
Contributor

@umpc umpc commented Apr 27, 2018

Hello,

First, thank you all so much for your work on Hyper! I had stopped using version 1 after the UI would freeze from attempting to display a large amount of output. I was so glad to see the switch to using a canvas element.

This PR addresses another occurrence of issue #401, where the session reference is undefined:

screenshot 2018-04-26 23 10 03

A user mistake that causes a large amount of output, leading to a session exit event, can cause Hyper to crash.

I suspect that the RPC handler for session exit events and its map delete call can, in rare cases, be scheduled to run before the final data event handler associated with its uid runs, which tries to call write on the now undefined session reference.

I could see where PR #404 added a conditional to catch this issue but was closed, though I did find its change in another RPC event handler within the same file added at a different time, attempting to prevent a crash in an exit event handler involving the same undefined session reference.

I was using version 2.0.0-stable on macOS 10.13 when this occurred. I was purposely fuzzing Hyper with /dev/zero and /dev/urandom output to test its reliability in handling large amounts of output. I triggered this using well-known output within the first hour of having Hyper installed.

I could not reproduce this issue on-demand, though the cause of the problem seems apparent considering the error in the screenshot, the previous issue/PR, and the existing conditional check of a session reference that calls console.log.

This PR removes the single console.log call and does not add them to the two new conditionals. I mention this as perhaps you would like to keep them there to make a different fix later, though I left them out as it's been nearly two years and I would guess that the proper fix isn't worth the extra code or design changes when a total of three conditionals, with one already present in the codebase, should resolve the crash bugs simply and with no side-effects. If this is acceptable, then this PR is ready to be merged.

I know that this issue was closed before, with a more satisfying fix desired, though the problem can occur in some rare but possible situations, and crash bugs affecting all tabs and windows were my only issue with using Hyper previously on a daily-basis. That is why I have been so thorough in explaining this now.

Thanks again for writing Hyper!

@chabou
Copy link
Contributor

chabou commented Apr 27, 2018

I am satisfied with this solution.
Fixes #2861 too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants