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

Fix race condition in Whirly.stop #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Thalagyrt
Copy link

Hiya! I've run across this one a few times using this, there's a race condition in the stop method that can cause unrender to throw an exception.

I see there's two failing tests in this PR, but they are failing on master as well.

  • Thread#terminate is not synchronous, it only schedules the thread
    to be killed.
  • The thread in the background can on rare occasions unset
    @current_frame after line 1 of Whirly.unrender, causing
    a nil to be passed to Unicode::DisplayWidth.of, and causing
    an exception as a result.
  • When we kill the thread, we need to join it to wait for it to
    finish before proceeding, otherwise we risk memory corruption.

- Thread#terminate is not synchronous, it only schedules the thread
  to be killed.
- The thread in the background can on rare occasions unset
  @current_frame after line 1 of Whirly.unrender, causing
  a nil to be passed to Unicode::DisplayWidth.of, and causing
  an exception as a result.
- When we kill the thread, we need to join it to wait for it to
  finish before proceeding, otherwise we risk memory corruption.
@Thalagyrt
Copy link
Author

Thalagyrt commented Mar 7, 2019

Just a note, I'm only 99.9% sure on this fix. It's exceedingly rare that the race condition hits, but all the symptoms line up with a thread race condition given how Thread#terminate works. I'll definitely have more info in about a week after people have had some time with it, we use this internally a lot and someone will for sure run into it and let me know if it's still happening sooner or later.

Stack trace for posterity:

Traceback (most recent call last):
<snip internal stuff>
         2: from /usr/local/bundle/gems/whirly-0.2.6/lib/whirly.rb:187:in `stop'
         1: from /usr/local/bundle/gems/whirly-0.2.6/lib/whirly.rb:208:in `unrender'
/usr/local/bundle/gems/unicode-display_width-1.4.1/lib/unicode/display_width.rb:9:in `of': undefined method `codepoints' for nil:NilClass (NoMethodError)

This specific exception hits if you call Unicode::DisplayWidth.of(nil), which led me to look at the threading implementation, since Whirly.unrender at a glance looks completely safe from passing a nil, unless a thread modifies the instance in parallel.

Re: Thread#terminate, this behavior is demonstrated here:

irb(main):001:0> t = Thread.new { sleep 10 }; t.terminate; puts t.inspect
#<Thread:0x00007fbdf00c6e70@(irb):1 run>
=> nil
irb(main):002:0> t = Thread.new { sleep 10 }; t.terminate; t.join; puts t.inspect
#<Thread:0x00007fbdee814d78@(irb):2 dead>
=> nil

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

Successfully merging this pull request may close these issues.

None yet

2 participants