Skip to content

Commit

Permalink
Unfreeze threads for object-evaluating commands
Browse files Browse the repository at this point in the history
There are several commands that evaluate objects and call methods on them,
such as:
- info
- ls (outline)
- trace object
- display

If the called method acquires a mutex shared with another thread (e.g.
when using Timeout), then it'd cause a deadlock as all threads are stopped.

For example, if there's an ActiveRecord Relation object stored as a local
variable, and the user runs the `info` command, then it'd call `inspect`
on it and trigger a database query, it could cause a deadlock as described
in ruby#877.

This commit fixes the issue by unfreezing all threads before evaluating
those commands and freezing them again after receiving the result event.
  • Loading branch information
st0012 committed Oct 26, 2023
1 parent dea10b5 commit c1ec064
Show file tree
Hide file tree
Showing 4 changed files with 113 additions and 14 deletions.
32 changes: 19 additions & 13 deletions lib/debug/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,11 @@ def request_tc(req)
@tc << req
end

def request_tc_with_freed_threads(req)
restart_all_threads
request_tc(req)
end

def process_event evt
# variable `@internal_info` is only used for test
tc, output, ev, @internal_info, *ev_args = evt
Expand Down Expand Up @@ -314,7 +319,7 @@ def process_event evt
if @displays.empty?
wait_command_loop
else
request_tc [:eval, :display, @displays]
request_eval :display, @displays
end
when :result
raise "[BUG] not in subsession" if @subsession_stack.empty?
Expand All @@ -329,6 +334,7 @@ def process_event evt
end
end

stop_all_threads
when :method_breakpoint, :watch_breakpoint
bp = ev_args[1]
if bp
Expand All @@ -342,6 +348,7 @@ def process_event evt
obj_inspect = ev_args[2]
opt = ev_args[3]
add_tracer ObjectTracer.new(@ui, obj_id, obj_inspect, **opt)
stop_all_threads
else
stop_all_threads
end
Expand Down Expand Up @@ -810,15 +817,15 @@ def register_default_command

case sub
when nil
request_tc [:show, :default, pat] # something useful
request_tc_with_freed_threads [:show, :default, pat] # something useful
when :locals
request_tc [:show, :locals, pat]
request_tc_with_freed_threads [:show, :locals, pat]
when :ivars
request_tc [:show, :ivars, pat, opt]
request_tc_with_freed_threads [:show, :ivars, pat, opt]
when :consts
request_tc [:show, :consts, pat, opt]
request_tc_with_freed_threads [:show, :consts, pat, opt]
when :globals
request_tc [:show, :globals, pat]
request_tc_with_freed_threads [:show, :globals, pat]
when :threads
thread_list
:retry
Expand All @@ -838,7 +845,7 @@ def register_default_command
# * Show you available methods and instance variables of the given object.
# * If the object is a class/module, it also lists its constants.
register_command 'outline', 'o', 'ls', unsafe: false do |arg|
request_tc [:show, :outline, arg]
request_tc_with_freed_threads [:show, :outline, arg]
end

# * `display`
Expand All @@ -848,9 +855,9 @@ def register_default_command
register_command 'display', postmortem: false do |arg|
if arg && !arg.empty?
@displays << arg
request_tc [:eval, :try_display, @displays]
request_eval :try_display, @displays
else
request_tc [:eval, :display, @displays]
request_eval :display, @displays
end
end

Expand All @@ -864,7 +871,7 @@ def register_default_command
if @displays[n = $1.to_i]
@displays.delete_at n
end
request_tc [:eval, :display, @displays]
request_eval :display, @displays
when nil
if ask "clear all?", 'N'
@displays.clear
Expand Down Expand Up @@ -983,7 +990,7 @@ def register_default_command
:retry

when /\Aobject\s+(.+)/
request_tc [:trace, :object, $1.strip, {pattern: pattern, into: into}]
request_tc_with_freed_threads [:trace, :object, $1.strip, {pattern: pattern, into: into}]

when /\Aoff\s+(\d+)\z/
if t = @tracers.values[$1.to_i]
Expand Down Expand Up @@ -1165,8 +1172,7 @@ def process_command line
end

def request_eval type, src
restart_all_threads
request_tc [:eval, type, src]
request_tc_with_freed_threads [:eval, type, src]
end

def step_command type, arg
Expand Down
32 changes: 32 additions & 0 deletions test/console/info_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -367,4 +367,36 @@ def test_ivar_abbrev
end
end
end

class InfoThreadLockingTest < ConsoleTestCase
def program
<<~RUBY
1| th0 = Thread.new{sleep}
2| $m = Mutex.new
3| th1 = Thread.new do
4| $m.lock
5| sleep 1
6| $m.unlock
7| end
8|
9| o = Object.new
10| def o.inspect
11| $m.lock
12| "foo"
13| end
14|
15| sleep 0.5
16| debugger
RUBY
end

def test_info_doesnt_cause_deadlock
debug_code(program) do
type 'c'
type 'info'
assert_line_text(/%self = main/)
type 'c'
end
end
end
end
31 changes: 31 additions & 0 deletions test/console/outline_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,35 @@ def test_outline_aliases
end
end
end

class OutlineThreadLockingTest < ConsoleTestCase
def program
<<~RUBY
1| th0 = Thread.new{sleep}
2| $m = Mutex.new
3| th1 = Thread.new do
4| $m.lock
5| sleep 1
6| $m.unlock
7| end
8|
9| def self.constants # overriding constants is only one of the ways to cause deadlock with outline
10| $m.lock
11| []
12| end
13|
14| sleep 0.5
15| debugger
RUBY
end

def test_outline_doesnt_cause_deadlock
debug_code(program) do
type 'c'
type 'ls'
assert_line_text(/locals: th0/)
type 'c'
end
end
end
end
32 changes: 31 additions & 1 deletion test/console/trace_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,36 @@ def test_block_doesnt_break_tracer
end
end

class ThreadLockingTest < ConsoleTestCase
def program
<<~RUBY
1| th0 = Thread.new{sleep}
2| $m = Mutex.new
3| th1 = Thread.new do
4| $m.lock
5| sleep 1
6| $m.unlock
7| end
8|
9| def inspect
10| m.lock
11| ""
12| end
13|
14| sleep 0.5
15| debugger
RUBY
end

def test_object_tracer_doesnt_cause_deadlock
debug_code(program) do
type 'c'
type 'trace object self'
type 'c'
end
end
end

class TraceCallReceiverTest < ConsoleTestCase
def program
<<~RUBY
Expand Down Expand Up @@ -544,7 +574,7 @@ def program
6| p a
RUBY
end

def test_1656237686
debug_code(program) do
type 'trace line'
Expand Down

0 comments on commit c1ec064

Please sign in to comment.