Bug #7513

TracePoint#enable/disable should not cause error

Added by Koichi Sasada over 2 years ago. Updated over 2 years ago.

[ruby-core:50561]
Status:Closed
Priority:Normal
Assignee:Koichi Sasada
ruby -v:ruby 2.0.0dev (2012-12-01 trunk 38127) [i386-mswin32_100] Backport:

Description

=begin
= Abstract

TracePoint#enable/disable should not cause error if it is enabled or disabled.

= Problem

The following code cause error because it calls "enable" on enabled tracepoint.

trace = TracePoint.trace{}
p trace.enabled? #=> true
trace.enable #=> `enable': trace is already enable (RuntimeError)

However, similar feature "GC.enable" and "GC.disable" don't cause error.
They only return previous status.

= Solution

TracePoint#enable and disable should align GC.enable/disable.

trace = TracePoint.trace{} # enable
trace.enable # do nothing, and return true (enabled)
trace.enable{
...
} # after block, trace is still enable

Any comments?

=end

Associated revisions

Revision 38227
Added by Koichi Sasada over 2 years ago

  • vm_trace.c: TracePoint#enable should not cause an error when it is already enabled. TracePoint#disable is too. [ruby-trunk - Bug #7513]
  • test/ruby/test_settracefunc.rb: add tests.

Revision 38227
Added by Koichi Sasada over 2 years ago

  • vm_trace.c: TracePoint#enable should not cause an error when it is already enabled. TracePoint#disable is too. [ruby-trunk - Bug #7513]
  • test/ruby/test_settracefunc.rb: add tests.

History

#1 Updated by Koichi Sasada over 2 years ago

(2012/12/05 11:52), ko1 (Koichi Sasada) wrote:

TracePoint#enable/disable should not cause error if it is enabled or disabled.

Patch:


Index: ChangeLog
===================================================================
--- ChangeLog (revision 38200)
+++ ChangeLog (working copy)
@@ -1,3 +1,11 @@
+Wed Dec 5 12:45:30 2012 Koichi Sasada ko1@atdot.net
+
+ * vm_trace.c: TracePoint#enable should not cause an error
+ when it is already enabled. TracePoint#disable is too.
+ [Bug #7513]
+
+ * test/ruby/test_settracefunc.rb: fix tests.
+
Wed Dec 5 11:42:38 2012 Koichi Sasada ko1@atdot.net

* test/ruby/test_settracefunc.rb: disable trace.

Index: vm_trace.c
===================================================================
--- vm_trace.c (revision 38199)
+++ vm_trace.c (working copy)
@@ -952,17 +952,20 @@ rb_tracepoint_disable(VALUE tpval)

/*
* call-seq:
- * trace.enable -> trace
+ * trace.enable -> bool
* trace.enable { block } -> obj
*
* Activates the trace
*
- * Will raise a RuntimeError if the trace is already activated
+ * Return true if trace was enabled.
+ * Return false if trace was disabled.
*
* trace.enabled? #=> false
- * trace.enable #=> #TracePoint:0x007fa3fad4aaa8
+ * trace.enable #=> false (previous state)
+ * # trace is enabled
* trace.enabled? #=> true
- * trace.enable #=> RuntimeError
+ * trace.enable #=> true (previous state)
+ * # trace is still enabled
*
* If a block is given, the trace will only be enabled within the scope
of the
* block. Note: You cannot access event hooks within the block.
@@ -986,17 +989,16 @@ static VALUE
tracepoint_enable_m(VALUE tpval)
{
rb_tp_t *tp = tpptr(tpval);
-
- if (tp->tracing) {
- rb_raise(rb_eRuntimeError, "trace is already enable");
- }
-
+ int previous_tracing = tp->tracing;
rb_tracepoint_enable(tpval);
+
if (rb_block_given_p()) {
- return rb_ensure(rb_yield, Qnil, rb_tracepoint_disable, tpval);
+ return rb_ensure(rb_yield, Qnil,
+ previous_tracing ? rb_tracepoint_enable : rb_tracepoint_disable,
+ tpval);
}
else {
- return tpval;
+ return previous_tracing ? Qtrue : Qfalse;
}
}

@@ -1007,12 +1009,13 @@ tracepoint_enable_m(VALUE tpval)
*
* Deactivates the trace
*
- * Will raise a RuntimeError if the trace is already deactivated
+ * Return true if trace was enabled.
+ * Return false if trace was disabled.
*
* trace.enabled? #=> true
- * trace.disable #=> #TracePoint:0x007fa3fad4aaa8
+ * trace.disable #=> false (previous status)
* trace.enabled? #=> false
- * trace.disable #=> RuntimeError
+ * trace.disable #=> false
*
* If a block is given, the trace will only be disable within the scope
of the
* block. Note: You cannot access event hooks within the block.
@@ -1028,25 +1031,21 @@ tracepoint_enable_m(VALUE tpval)
* trace.enabled?
* #=> true
*
- * trace.enable { p trace.lineno }
- * #=> RuntimeError: access from outside
- *
*/
static VALUE
tracepoint_disable_m(VALUE tpval)
{
rb_tp_t *tp = tpptr(tpval);
-
- if (!tp->tracing) {
- rb_raise(rb_eRuntimeError, "trace is not enable");
- }
-
+ int previous_tracing = tp->tracing;
rb_tracepoint_disable(tpval);
+
if (rb_block_given_p()) {
- return rb_ensure(rb_yield, Qnil, rb_tracepoint_enable, tpval);
+ return rb_ensure(rb_yield, Qnil,
+ previous_tracing ? rb_tracepoint_enable : rb_tracepoint_disable,
+ tpval);
}
else {
- return tpval;
+ return previous_tracing ? Qtrue : Qfalse;
}
}

Index: test/ruby/test_settracefunc.rb
===================================================================
--- test/ruby/test_settracefunc.rb (revision 38200)
+++ test/ruby/test_settracefunc.rb (working copy)
@@ -619,6 +619,16 @@ class TestSetTraceFunc < Test::Unit::Tes
}
foo
assert_equal([:foo], ary)
+
+ trace = TracePoint.new{}
+ begin
+ assert_equal(false, trace.enable)
+ assert_equal(true, trace.enable)
+ trace.enable{}
+ assert_equal(true, trace.enable)
+ ensure
+ trace.disable
+ end
end

def test_tracepoint_disable

@@ -633,6 +643,14 @@ class TestSetTraceFunc < Test::Unit::Tes
foo
trace.disable
assert_equal([:foo, :foo], ary)
+
+ trace = TracePoint.new{}
+ trace.enable{
+ assert_equal(true, trace.disable)
+ assert_equal(false, trace.disable)
+ trace.disable{}
+ assert_equal(false, trace.disable)
+ }
end

def test_tracepoint_enabled

--
// SASADA Koichi at atdot dot net

#2 Updated by Zachary Scott over 2 years ago

=begin
Hi Koichi-san,

For boolean call-seq, I like: trace.enable -> true or false

Re: calling event hooks within enable/disable block.

I mean here: (({trace.enable { p trace.lineno } #=> RuntimeError: access from outside}))
=end

#3 Updated by Koichi Sasada over 2 years ago

  • % Done changed from 0 to 100
  • Status changed from Open to Closed

This issue was solved with changeset r38227.
Koichi, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • vm_trace.c: TracePoint#enable should not cause an error when it is already enabled. TracePoint#disable is too. [ruby-trunk - Bug #7513]
  • test/ruby/test_settracefunc.rb: add tests.

#4 Updated by Koichi Sasada over 2 years ago

  • Status changed from Closed to Feedback

#5 Updated by Koichi Sasada over 2 years ago

(2012/12/06 4:56), zzak (Zachary Scott) wrote:

For boolean call-seq, I like: trace.enable -> true or false

Okay.

Re: calling event hooks within enable/disable block.

I mean here: (({trace.enable { p trace.lineno } #=> RuntimeError: access from outside}))

Thank you. It makes sense.

I'll commit it because there are no compatible issue.
If you try and feel strange, please comment us.

--
// SASADA Koichi at atdot dot net

#6 Updated by Zachary Scott over 2 years ago

Thanks koichi, this is much better.

#7 Updated by Koichi Sasada over 2 years ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF