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

Avoid setting @tc in thread_begin event #727

Merged
merged 1 commit into from
Aug 10, 2022
Merged

Avoid setting @tc in thread_begin event #727

merged 1 commit into from
Aug 10, 2022

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jul 21, 2022

This event will be triggered before the new thread even has a ThreadClient. So in this case, the @tc will be overridden to just nil,
which is wrong.

However, we also don't want to just set @tc to the new thread's newly created ThreadClient. This is because a thread's creation can happen during a subsession, and promptly switching @tc to another thread's ThreadClient will misfire thread suspension.

Fixes #725

@ko1
Copy link
Collaborator

ko1 commented Jul 27, 2022

00ec229
It's my fault can not checking this bug.

@ko1
Copy link
Collaborator

ko1 commented Jul 27, 2022

Could you add a test program or repro- small code?

@ko1
Copy link
Collaborator

ko1 commented Jul 27, 2022

I'm not sure the issue and why the patch solves yet.

  1. Stop at TC1: @tc = TC1
  2. eval Thread.new{} on TC1
  3. receive thread_begin and @tc = nil
  4. receive result event and @tc = T1 (maybe)

In this case, there is no issue. Could you describe what's happens?

@ko1
Copy link
Collaborator

ko1 commented Jul 27, 2022

@ko1
Copy link
Collaborator

ko1 commented Jul 27, 2022

Another threads can resume by some events so that current code is unstable for that even if the patch is merged.
Anyway I'll merge it (and modify them soon) if you wrote can write a test code.

This event will be triggered before the new thread even has a
ThreadClient. So in this case, the @tc will be overridden to just nil,
which is wrong.

However, we also don't want to just set @tc to the new thread's
newly created ThreadClient. This is because a thread's creation can
happen during a subsession, and promptly switching @tc to another
thread's ThreadClient will misfire thread suspension.
@st0012
Copy link
Member Author

st0012 commented Jul 28, 2022

I've added a test that'd hang and timeout on master.

@ianterrell
Copy link

@ko1 @st0012 — Thanks so much for this fix, it really helps our workflows. I expect this is impacting a lot of Rails apps in the wild. Is there anything else I can do to help get this merged in and released?

@ko1 ko1 merged commit 3fdcfef into ruby:master Aug 10, 2022
@st0012 st0012 deleted the fix-#725 branch August 10, 2022 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock on executing commands from binding.b in Rails app
3 participants