Skip to content

Commit 4c9c40a

Browse files
committed
fix: Eval handler now works correctly even when not tracing
Eval handler relies on specific call stack layout when recovering original eval call context. Since call hook handler used to short-circuit over trace_call method when not tracing, the layout was unexpected when the handlers were loaded but tracing disabled (eg. when initial code load is performed dynamically after loading appmap gem). The fix is to always call trace_call and short-circuit in there, keeping the call stack the same. Alternative, rejected solutions: - check for tracing being disabled in eval handler and use the correct (shallower) call stack depth in this case, - examine all frames sequentially in the eval handler until the correct one is found.
1 parent ae41d85 commit 4c9c40a

File tree

4 files changed

+34
-6
lines changed

4 files changed

+34
-6
lines changed

lib/appmap/hook/method/ruby2.rb

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,9 @@ class Hook
88
# cf. https://eregon.me/blog/2019/11/10/the-delegation-challenge-of-ruby27.html
99
class Method
1010
ruby2_keywords def call(receiver, *args, &block)
11-
return do_call(receiver, *args, &block) unless trace?
12-
13-
call_event = with_disabled_hook { before_hook receiver, *args }
11+
call_event = trace? && with_disabled_hook { before_hook receiver, *args }
12+
# note we can't short-circuit directly to do_call because then the call stack
13+
# depth changes and eval handler doesn't work correctly
1414
trace_call call_event, receiver, *args, &block
1515
end
1616

@@ -32,7 +32,10 @@ def before_hook(receiver, *args)
3232
hook_method.bind(receiver).call(*args, &block)
3333
end
3434

35+
# rubocop:disable Metrics/MethodLength
3536
ruby2_keywords def trace_call(call_event, receiver, *args, &block)
37+
return do_call(receiver, *args, &block) unless call_event
38+
3639
start_time = gettime
3740
begin
3841
return_value = do_call(receiver, *args, &block)
@@ -44,6 +47,7 @@ def before_hook(receiver, *args)
4447
if call_event
4548
end
4649
end
50+
# rubocop:enable Metrics/MethodLength
4751

4852
def hook_method_def
4953
this = self

lib/appmap/hook/method/ruby3.rb

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,9 @@ class Hook
55
# Delegation methods for Ruby 3.
66
class Method
77
def call(receiver, *args, **kwargs, &block)
8-
return do_call(receiver, *args, **kwargs, &block) unless trace?
9-
10-
call_event = with_disabled_hook { before_hook receiver, *args, **kwargs }
8+
call_event = trace? && with_disabled_hook { before_hook receiver, *args, **kwargs }
9+
# note we can't short-circuit directly to do_call because then the call stack
10+
# depth changes and eval handler doesn't work correctly
1111
trace_call call_event, receiver, *args, **kwargs, &block
1212
end
1313

@@ -34,7 +34,10 @@ def do_call(receiver, *args, **kwargs, &block)
3434
hook_method.bind_call(receiver, *args, **kwargs, &block)
3535
end
3636

37+
# rubocop:disable Metrics/MethodLength
3738
def trace_call(call_event, receiver, *args, **kwargs, &block)
39+
return do_call(receiver, *args, **kwargs, &block) unless call_event
40+
3841
start_time = gettime
3942
begin
4043
return_value = do_call(receiver, *args, **kwargs, &block)
@@ -46,6 +49,7 @@ def trace_call(call_event, receiver, *args, **kwargs, &block)
4649
if call_event
4750
end
4851
end
52+
# rubocop:enable Metrics/MethodLength
4953

5054
def hook_method_def
5155
this = self

spec/handler/class_with_eval.rb

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
# frozen_string_literal: true
2+
3+
# rubocop:disable Style/EvalWithLocation
4+
5+
module AppMap
6+
class SpecClasses
7+
class WithEval
8+
eval %(def text; 'text'; end)
9+
end
10+
end
11+
end
12+
13+
# rubocop:enable Style/EvalWithLocation

spec/handler/eval_spec.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,17 @@ def record_block
5757
end
5858
expect(new_cls.const_get(class_name)).to eq(cls)
5959
end
60+
61+
it 'works correctly when loaded even when not tracing' do
62+
load "#{__dir__}/class_with_eval.rb"
63+
expect { AppMap::SpecClasses::WithEval.new.text }.to_not raise_error(NameError)
64+
end
6065
end
6166

6267
module ClassMaker
6368
def self.make_class(class_name)
6469
eval "class #{class_name}; end; #{class_name}"
6570
end
6671
end
72+
73+
# rubocop:enable Security/Eval, Style/EvalWithLocation

0 commit comments

Comments
 (0)