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

Only apply TracePoint to current thread #759

Merged
merged 1 commit into from May 27, 2018

Conversation

arkadiyt
Copy link
Contributor

@arkadiyt arkadiyt commented May 20, 2018

Hi,

While upgrading to webmock 3.4.0 I found that some of my specs started failing. The way my tests are setup, in addition to using webmock for stubbing out requests in unit tests I also perform integration tests by making unmocked http requests against a WEBRick server running in a different thread in my rspec process.

After a bit of debugging I've narrowed the issue down to the TracePoint code added in #751 not being thread safe. In particular that code will undefine any local variable called tmp whenever any line of code is run in any thread, even not in the rbuf_fill thread for which it was intended.

The end result is I would get exceptions in places like webrick/log.rb#L152:

    def log(level, data)
      tmp = Time.now.strftime(@time_format)
      tmp << " " << data  # Throws exception here
      super(level, tmp)
    end

With errors on tmp << " " like undefined method '<<' for nil:NilClass (NoMethodError) (despite tmp having been immediately defined the line before).

Or webrick/httputils.rb#L211:

    def parse_qvalues(value)
      tmp = []
      if value
        parts = value.split(/,\s*/)
        parts.each {|part|
          if m = %r{^([^\s,]+?)(?:;\s*q=(\d+(?:\.\d+)?))?$}.match(part)
            val = m[1]
            q = (m[2] or 1).to_f
            tmp.push([val, q])  # Throws nil exception for undefined tmp here

My fix is to apply the tracepoint only to the rbuf_fill thread - after applying this change locally all my tests pass without exception.

Thanks

edit: I believe the jruby-9.0.5.0 failure on travis is unrelated to my change

@arkadiyt arkadiyt force-pushed the tracepoint-fix branch 5 times, most recently from fddb599 to d45f108 Compare May 20, 2018 19:48
@bblimke
Copy link
Owner

bblimke commented May 27, 2018

@arkadiyt Thank you for this fix!

@bblimke bblimke merged commit 11feb72 into bblimke:master May 27, 2018
@arkadiyt
Copy link
Contributor Author

arkadiyt commented Jun 1, 2018

@bblimke thanks for merging - would it be possible to cut a new patch release for this?

@bblimke
Copy link
Owner

bblimke commented Jun 3, 2018

@arkadiyt Done. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants