Project

General

Profile

Actions

Bug #16889

open

TracePoint.enable { ... } also activates the TracePoint for other threads, even outside the block

Added by Eregon (Benoit Daloze) over 1 year ago. Updated about 2 months ago.

Status:
Open
Priority:
Normal
Target version:
-
[ruby-core:98355]

Description

threads = []
inspects = []
trace = TracePoint.new(:line) do |tp|
  threads << Thread.current
  inspects << tp.inspect
end

done = false
thread = Thread.new do
  Thread.pass until done
end

trace.enable do
  line_event = true
  done = true
  sleep 1
end
thread.join

# Expected only within enable block (lines 14-16)
puts inspects

# Expected just 1
p threads.uniq

Results in:

$ ruby tpbug.rb
ruby tpbug.rb
#<TracePoint:line@tpbug.rb:14>
#<TracePoint:line@tpbug.rb:15>
#<TracePoint:line@tpbug.rb:16>
#<TracePoint:line@tpbug.rb:10>
[#<Thread:0x00005571134e3340 run>, #<Thread:0x00005571138ac828@tpbug.rb:9 dead>]

But I expected:

#<TracePoint:line@tpbug.rb:14>
#<TracePoint:line@tpbug.rb:15>
#<TracePoint:line@tpbug.rb:16>
[#<Thread:0x00005571134e3340 run>]

Because the RDoc says:

If a block is given, the trace will only be enabled within the scope of
the block.

For background I'm trying to improve the TracePoint specs in ruby/spec, but they are proving quite unreliable due to this.

ko1 (Koichi Sasada) Thoughts?

Actions #1

Updated by Eregon (Benoit Daloze) over 1 year ago

  • Description updated (diff)

Updated by Eregon (Benoit Daloze) over 1 year ago

Maybe enable(&block) should behave like enable(target: block); disable?

Updated by jeremyevans0 (Jeremy Evans) 4 months ago

  • Backport deleted (2.5: UNKNOWN, 2.6: UNKNOWN, 2.7: UNKNOWN)
  • ruby -v deleted (ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-linux])
  • Tracker changed from Bug to Feature

Eregon (Benoit Daloze) wrote in #note-2:

Maybe enable(&block) should behave like enable(target: block); disable?

I looked into this. Implementing this fairly simple, but it breaks many tests and specs. The reason for this is that there are many tests and specs where this is not desired behavior, because they don't want to test tracing of the block itself, but tracing during the block.

Most of the breakage is actually that calling TracePoint#enable in a way that doesn't actually add any traces raises an error. Here's one such case, taken from test_tracepoint_thread:

    events = []
    thread_self = nil
    created_thread = nil
    TracePoint.new(:thread_begin, :thread_end){|tp|
      events << [Thread.current,
                 tp.event,
                 tp.lineno,  #=> 0
                 tp.path,    #=> nil
                 tp.binding, #=> nil
                 tp.defined_class, #=> nil,
                 tp.self.class # tp.self return creating/ending thread
                 ]
    }.enable{
      created_thread = Thread.new{thread_self = self}
      created_thread.join
    }
    events.reject!{|i| i[0] != created_thread}

Automatically making the block the target, similar to this code, results in an error:

    events = []
    thread_self = nil
    created_thread = nil
    block = proc{
      created_thread = Thread.new{thread_self = self}
      created_thread.join
    }
    TracePoint.new(:thread_begin, :thread_end){|tp|
      events << [Thread.current,
                 tp.event,
                 tp.lineno,  #=> 0
                 tp.path,    #=> nil
                 tp.binding, #=> nil
                 tp.defined_class, #=> nil,
                 tp.self.class # tp.self return creating/ending thread
                 ]
    }.enable(target: block, &block)
    # <internal:trace_point>:199:in `enable': can not enable any hooks (ArgumentError)

I'm guessing this is because :thread_begin and :thread_end events aren't triggered for code targets, unlike :line events.

Due to this issue, I don't think it makes sense to turn enable(&block) into enable(target: block); disable by default.

However, I think there value in making it nicer to enable only the targeting of the block. You can do this already, but it is clunky. In your example, you would do it like this:

threads = []
inspects = []
trace = TracePoint.new(:line) do |tp|
  threads << Thread.current
  inspects << tp.inspect
end

done = false
thread = Thread.new do
  Thread.pass until done
end

block = proc do
  line_event = true
  done = true
  sleep 1
end
trace.enable(target: block, &block)
thread.join

# Expected only within enable block (lines 14-16)
puts inspects

# Expected just 1
p threads.uniq

I propose we support target: :block to support using the block passed to enable as the target. This would enable the following API:

threads = []
inspects = []
trace = TracePoint.new(:line) do |tp|
  threads << Thread.current
  inspects << tp.inspect
end

done = false
thread = Thread.new do
  Thread.pass until done
end

trace.enable(target: :block) do
  line_event = true
  done = true
  sleep 1
end
thread.join

# Expected only within enable block (lines 14-16)
puts inspects

# Expected just 1
p threads.uniq

I've submitted a pull request that implements this: https://github.com/ruby/ruby/pull/4624.

Actions #4

Updated by Eregon (Benoit Daloze) 3 months ago

The reason for this is that there are many tests and specs where this is not desired behavior, because they don't want to test tracing of the block itself, but tracing during the block.

Right, I guess the expectation is that .enable { ... } also reports events from calls inside that block.
Maybe .enable { ... } should enable events only on that Thread (or better, Fiber) then?
That feels intuitive and I think is good enough for compatibility, what do you think?

Updated by Eregon (Benoit Daloze) 3 months ago

trace.enable(target: :block) seems nice to me (WDYT ko1 (Koichi Sasada)?).

But I think trace.enable { ... } should not affect other Threads/Fiber, otherwise that form is just too confusing and error-prone.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

Eregon (Benoit Daloze) wrote in #note-5:

But I think trace.enable { ... } should not affect other Threads/Fiber, otherwise that form is just too confusing and error-prone.

If you look at the tests, they expect that enable is not limited to the current thread. The expected usage is tracing other threads during the duration of the block. You can already specify to only trace the current thread using: enable(target_thread: Thread.current). I do think enable only affecting the current thread may be a more common desire, but I can also see that enable allowing tracing other threads inside the block is also a reasonable use. I don't think it's worth breaking backwards compatibility to change the behavior.

Updated by mame (Yusuke Endoh) 2 months ago

TracePoint#enable(target: :block) does not follow events in a called method. Is this really intentional?

trace = TracePoint.new(:line) {|tp| p tp }

def foo
  1
end

trace.enable do
  foo
end
#=>
# #<TracePoint:line t.rb:8>
# #<TracePoint:line t.rb:4 in `foo'>

puts

trace.enable(target: :block) do
  foo
end
#=>
# #<TracePoint:line t.rb:17>

Note that the second trace.enable does not print #<TracePoint:line t.rb:4 in 'foo'>.

Updated by ko1 (Koichi Sasada) 2 months ago

+1 for Jeremy #6.

I also agree TP#enable{ ... } should be thread-local without any options, but I don't think it is valuable to change the default behaver (break compatibility).

Updated by jeremyevans0 (Jeremy Evans) 2 months ago

mame (Yusuke Endoh) wrote in #note-7:

TracePoint#enable(target: :block) does not follow events in a called method. Is this really intentional?

trace = TracePoint.new(:line) {|tp| p tp }

def foo
  1
end

trace.enable do
  foo
end
#=>
# #<TracePoint:line t.rb:8>
# #<TracePoint:line t.rb:4 in `foo'>

puts

trace.enable(target: :block) do
  foo
end
#=>
# #<TracePoint:line t.rb:17>

Note that the second trace.enable does not print #<TracePoint:line t.rb:4 in 'foo'>.

This matches the behavior of passing the block explicitly and using it as the target:

trace = TracePoint.new(:line) {|tp| p tp }

def foo
  1
end

trace.enable do
  foo
end
#=>
# #<TracePoint:line t.rb:8>
# #<TracePoint:line t.rb:4 in `foo'>

puts

pr = proc do
  foo
end

trace.enable(target: pr, &pr) 

#<TracePoint:line t.rb:17>

So I think it is expected. My guess would be while the location of the trace point shows the point of call, it isn't triggered unless the method itself is targeted. If you target both the method and the block, you get both lines:

trace = TracePoint.new(:line) {|tp| p tp }
trace2 = TracePoint.new(:line) {|tp| p tp }

def foo
  1
end

trace.enable do
  foo
end
#=>
# #<TracePoint:line t.rb:8>
# #<TracePoint:line t.rb:4 in `foo'>

puts

pr = proc do
  foo
end

trace2.enable(target: method(:foo)) do
  trace.enable(target: pr, &pr)
end
# #<TracePoint:line -:18>
# #<TracePoint:line -:5 in `foo'>

Whether the current behavior of targeted traces is a bug, I'm not sure. If so, it isn't a bug in the target: :block support, but a more general issue with targeted traces.

Updated by Eregon (Benoit Daloze) 2 months ago

mame (Yusuke Endoh) wrote in #note-7:

Note that the second trace.enable does not print #<TracePoint:line t.rb:4 in 'foo'>.

A very good point, that I thought about when talking a bit with Koichi about this, some time before filing this issue and then apparently forgot when filing the issue.

I do expect:

TracePoint.new(:line) {|tp| p tp }.enable do
  foo
end

to report lines within that last block, and within foo, so thread-local behavior in other words.
So I'd want tp.enable {} to be like tp.enable(target_thread: Thread.current) {}.

ko1 (Koichi Sasada) wrote in #note-8:

I also agree TP#enable{ ... } should be thread-local without any options, but I don't think it is valuable to change the default behaver (break compatibility).

Exactly. I think it could be worth it, because I think most people expect TP#enable {} to be thread local and not affect other threads which are executing in random places completely unrelated to the block.
The way I see it is enable { ... } should trace whatever is inside that block (including calls), but not other threads since they are not executing inside that block.
This is also why I filed it as a Bug, IMHO the current behavior is unexpected.

I guess the purpose of the current behavior is it behaves like open and so enable { ... } is like enable; ...; ensure; disable.
And maybe initially TracePoint did not support tracing a particular Thread?

I think it would be worth to experiment how little it would break to make tp.enable {} to be like tp.enable(target_thread: Thread.current) {}.
IMHO it would be worth changing, but I can also see others have compatibility concerns.
If someone actually wants to enable for all threads, they could anyway do tp.enable; ...; ensure; tp.disable, or tp.enable(target_thread: nil/:all/Thread.list).

If we keep the current behavior we should fix the docs, for me these docs clearly indicate thread-local behavior:

If a block is given, the trace will only be enabled within the scope of
the block.

  trace.enabled?
  #=> false

  trace.enable do
    trace.enabled?
    # only enabled for this block
  end

  trace.enabled?
  #=> false

i.e. "scope of the block" sounds like "events with this block call in the call stack" or "events within that block", but not the actual behavior "enable for all threads, and when quitting the block disable for all threads".


Regarding target: Proc I think it's clearly documented to only trace the Proc and no calls within it (rdoc of TracePoint#enable), so no objection to the behavior of enable(target:):

target, target_line and target_thread parameters are used to limit
tracing only to specified code objects. target should be a code object
for which RubyVM::InstructionSequence.of will return an instruction
sequence.

Updated by Eregon (Benoit Daloze) 2 months ago

Actually thread-local might not be enough, we'd probably want Fiber local, i.e., events which are on the same execution stack and contain that enable block call on the stack.
Is there any way to have a TracePoint for a particular Fiber?

Updated by ko1 (Koichi Sasada) about 2 months ago

Eregon (Benoit Daloze) wrote in #note-11:

Actually thread-local might not be enough, we'd probably want Fiber local, i.e., events which are on the same execution stack and contain that enable block call on the stack.
Is there any way to have a TracePoint for a particular Fiber?

Just now, it is not provided.

Updated by mame (Yusuke Endoh) about 2 months ago

Eregon (Benoit Daloze) wrote in #note-10:

I do expect:

TracePoint.new(:line) {|tp| p tp }.enable do
  foo
end

to report lines within that last block, and within foo, so thread-local behavior in other words.
So I'd want tp.enable {} to be like tp.enable(target_thread: Thread.current) {}.

Agreed. I think thread-local behavior is more reasonable. So, the current target: :block is not what you want, right?

Though I like thread-local behavior, I have some concerns:

  • tp.enable { Thread.new { foo }.join } will not trace foo because it is run in another thread, but I'm unsure if this is expected.
  • tp.enable called with no block should trace events in all threads, right? It is a bit inconsistent, but perhaps changing this to thread-local will break Zeitwerk.

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

  • Backport set to 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN
  • Tracker changed from Feature to Bug

This was discussed at the August 2021 developer meeting, and it was decided that TracePoint#enable with a block will be thread-local by default. However, that change will not take place until after Ruby 3.1. I'm switching this back to bug.

Updated by Eregon (Benoit Daloze) about 2 months ago

mame (Yusuke Endoh) wrote in #note-13:

Agreed. I think thread-local behavior is more reasonable. So, the current target: :block is not what you want, right?

Indeed. I'm not against it but I don't think it's necessary or so helpful.

Though I like thread-local behavior, I have some concerns:

  • tp.enable { Thread.new { foo }.join } will not trace foo because it is run in another thread, but I'm unsure if this is expected.

I think it is expected. foo is not on the call stack of that tp.enable (it does have that TracePoint#enable as a caller).
With that logic I would also prefer if tp.enable { Fiber.new { foo }.resume } does not trace foo, but it is less important, as that could only happen if the enable blocks switches Fibers (probably rare).

Currently, Thread.new { sleep 1; foo }; tp.enable { bar } will trace bar (expected) but also foo (I find that unexpected/unintuitive).

  • tp.enable called with no block should trace events in all threads, right? It is a bit inconsistent, but perhaps changing this to thread-local will break Zeitwerk.

Yes, tp.enable (no block) should trace events everywhere, I think that's intuitive, it doesn't provide any "target" or "context" (a block) so it's global.

Actions

Also available in: Atom PDF