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 locking all threads on debugger suspension #16

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Apr 20, 2023

This commit stops locking all threads during suspension, which may prevent users from investigating threading problem. But it's still better than freezing the process just by running a ActiveRecord query, or doing anything that calls Timeout.timeout.

History around this issue:

  1. It originally was reported in https://github.com/Shopify/ruby-dev-exp-issues/issues/724, and we reported it upstream too.
  2. We introduced 1e0f45c as a workaround.
  3. Upstream proposed a solution to fix the query evaluation case by restarting threads before evaluating console/DAP input. So the above commit was reverted.
  4. However, the process could still hang if Timeout.timeout is called: 1. During a suspension 2. And outside of input evaluation An example is typing Arrow-up in the console as reline calls Timeout.timeout underneath.
  5. So we need to reintroduce the workaround until we can find a better with the upstream maintainer.

This commit stops locking all threads during suspension, which may prevent
users from investigating threading problem. But it's still better than
freezing the process just by running a ActiveRecord query, or doing
anything that calls `Timeout.timeout`.

History around this issue:

1. It originally was reported in Shopify/team-ruby-dx#724,
   and we reported it upstream too.
1. We introduced 1e0f45c as a workaround.
1. Upstream proposed a solution to fix the query evaluation case by restarting
   threads before evaluating console/DAP input. So the above commit was
   reverted.
1. However, the process could still hang if `Timeout.timeout` is called:
     1. During a suspension
     2. And outside of input evaluation
   An example is typing Arrow-up in the console as `reline` calls `Timeout.timeout`
   underneath.
1. So we need to reintroduce the workaround until we can find a better
   with the upstream maintainer.
@st0012 st0012 self-assigned this Apr 20, 2023
@st0012 st0012 requested a review from a team April 20, 2023 17:24
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is anything lost by not stopping all threads? Or in other words, why isn't this the optimal solution?

@st0012
Copy link
Member Author

st0012 commented Apr 20, 2023

Yes it has some drawbacks:

  • In multi-thread scenario, we'll see the same messed up output when a breakpoint is hit by multiple threads. This is the same problem you'd see with Pry or IRB, for example.
  • Because debug assumes all threads are always stopped, there's no command to stop/unstop individual threads. This means with this patch, we don't have the ability to control individual threads and they'll always be running.

But since multi-thread support to us is not a significant, it's a tradeoff we can happily take to fix the serious issue.

That being said, I also asked if it's possible to make thread stopping configurable. And perhaps when configured as "not stopping threads by default", we can have commands to control individual threads.

@st0012 st0012 merged commit f1a45a9 into issues-workaround Apr 20, 2023
st0012 added a commit that referenced this pull request Apr 21, 2023
Without this change, threads would still be locked after evalution, which
would still cause the problem described in #16.
st0012 added a commit that referenced this pull request Apr 21, 2023
Otherwise, we'd still face the same problem as in #16, where the
debugger would hang because the timeout thread is locked.
st0012 added a commit that referenced this pull request Apr 21, 2023
In #16, I removed the thread locking on suspension, but I missed the one
that's after evaluation, which is what I comment out in this commit.

Otherwise, we'd still face the same problem as in #16, where the debugger
would hang because the timeout thread is locked.
st0012 added a commit that referenced this pull request Apr 21, 2023
In #16, I removed the thread locking on suspension, but I missed the one
that's after evaluation, which is what I comment out in this commit.

Otherwise, we'd still face the same problem as in #16, where the debugger
would hang because the timeout thread is locked.
@st0012 st0012 deleted the stop-thread-suspension branch April 21, 2023 21:41
st0012 added a commit that referenced this pull request Aug 25, 2023
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