Project

General

Profile

Actions

Feature #16889

open

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

Added by Eregon (Benoit Daloze) about 1 year ago. Updated 29 days 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) about 1 year ago

  • Description updated (diff)

Updated by Eregon (Benoit Daloze) about 1 year ago

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

Updated by jeremyevans0 (Jeremy Evans) about 1 month 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) 29 days 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) 29 days 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) 29 days 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.

Actions

Also available in: Atom PDF