[ruby-core:104948] [Ruby master Feature#16889] TracePoint.enable { ... } also activates the TracePoint for other threads, even outside the block

Issue #16889 has been updated by Eregon (Benoit Daloze).


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.

----------------------------------------
Feature #16889: TracePoint.enable { ... } also activates the TracePoint for other threads, even outside the block
https://bugs.ruby-lang.org/issues/16889#change-93312

* Author: Eregon (Benoit Daloze)
* Status: Open
* Priority: Normal
* Assignee: ko1 (Koichi Sasada)
----------------------------------------
```ruby
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:[e-mail]:14>
#<TracePoint:[e-mail]:15>
#<TracePoint:[e-mail]:16>
#<TracePoint:[e-mail]:10>
[#<Thread:0x00005571134e3340 run>, #<Thread:[e-mail]:9 dead>]
```

But I expected:
```
#<TracePoint:[e-mail]:14>
#<TracePoint:[e-mail]:15>
#<TracePoint:[e-mail]: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 Thoughts?



-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>