-
Notifications
You must be signed in to change notification settings - Fork 129
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
Restart threads on DAP evaluation #950
Conversation
Thank you for the report. But this patch is not acceptable because it doesn't stop other threads after the evaluation. _th0 = Thread.new{loop{sleep 0.5; p :th0}}
_th1 = Thread.new{loop{sleep 0.5; p :th1}}
loop do
sleep 0.5
p :main
end Please try the above code and evaluate something on the debug console. |
[master]$ git diff
diff --git a/lib/debug/server_dap.rb b/lib/debug/server_dap.rb
index 8298428..bae1e4d 100644
--- a/lib/debug/server_dap.rb
+++ b/lib/debug/server_dap.rb
@@ -625,6 +625,7 @@ module DEBUGGER__
expr = req.dig('arguments', 'expression')
if find_waiting_tc(tid)
+ restart_all_threads
request_tc [:dap, :evaluate, req, fid, expr, context]
else
fail_response req
@@ -701,6 +702,7 @@ module DEBUGGER__
register_vars result[:variables], tid
@ui.respond req, result
when :evaluate
+ stop_all_threads
message = result.delete :message
if message
@ui.respond req, success: false, message: message could you add |
This makes sure DAP evaluation request also doesn't hang. See ruby#947 for the original console implementation.
Ah I thought it's handled in |
d024286
to
076f56b
Compare
I think I found another issue: when I run the test, the result was not deterministic even without sleep and fails around 10% of the time on my machine. I initially thought it's because of my test, but then I found that it's because this early return isn't always correct. Sometimes it doesn't reflect the actual running thread count and thus the debugger doesn't stop the threads. If I comment it out, the test doesn't fail intermittently anymore. Should I address that in this PR too? |
Now current behavior is already non-deterministic (by design). |
As the test could make CI unstable, I've dropped it. |
This makes sure DAP evaluation request also doesn't hang.
See #947 for the original console implementation.