Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disable TracePoint properly #788

Merged
merged 1 commit into from Dec 27, 2018

Conversation

Projects
None yet
4 participants
@ko1
Copy link
Contributor

commented Dec 12, 2018

if super raise an exception, this very danger TracePoint is not disabled.
This is why https://bugs.ruby-lang.org/issues/15400

This change is introduced at: #751

Anyway, this approach is very danger (for example, tmp variable name is not exposed API), so that pls re-consider to use this technique.

disable TracePoint properly
if `super` raise an exception, this very danger TracePoint is not disabled.
This is why https://bugs.ruby-lang.org/issues/15400

This change is introduced at: #751

Anyway, this approach is very danger (for example, `tmp` variable name is not exposed API), so that pls re-consider to use this technique.
@@ -285,7 +285,7 @@ def rbuf_fill
end

super

ensure

This comment has been minimized.

Copy link
@hanachin

hanachin Dec 12, 2018

In Ruby 2.6, the TracePoint#enable takes target: keyword argument.
We can use TracePoint#enable with the target: to enable the TracePoint for the specific method.
What do you think?

        trace.enable(target: WebMock::HttpLibAdapters::NetHttpAdapter::OriginalNetBufferedIO.instance_method(:rbuf_fill)) do
          super
        end

This comment has been minimized.

Copy link
@ko1

ko1 Dec 12, 2018

Author Contributor

you == me?

It is better. But I think this TracePoint approach is danger, even with this limitation.

@bblimke

This comment has been minimized.

Copy link
Owner

commented Dec 12, 2018

Any thoughts of how to avoid using trace point to achieve the same behaviour?

@ko1

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Not sure because I don't know about net/protocol.rb, but modifying @rbuf? monkey patching is better than it because you need to check the behavior change (maybe you specify the version more explicitly).

@lamont-granquist

This comment has been minimized.

Copy link

commented Dec 26, 2018

ping? it might be better to remove tracepoint, but merging this would unblock our 2.6 testing.

@bblimke bblimke merged commit ddf3191 into bblimke:master Dec 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.