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

Change httpclient adapter to use thread local variables for thread safety #300

Merged
merged 1 commit into from
Aug 17, 2013

Conversation

tbeauvais
Copy link

This fixes a threading issue when doing load testing using webmock for our HttpClient service calls using JRuby. The httpclient_adapter was using shared instance variables which caused response corruption.

Oh I noticed before I even started that there were several spec failures. I looked it to it quickly and noticed that calls http://www.example.com/ now results in 200, with actual content. Is someone looking at that?

@bblimke
Copy link
Owner

bblimke commented Aug 7, 2013

Cheers. One question:

Is there a reason for using non http client specific global variable Thread.current[:webmock_responses]
instead of instance variable @webmock_responses[Thread.current.object_id]

@tbeauvais
Copy link
Author

Since that instance variable is shared across threads it's not safe to have simultaneous writes to an Array or Hash. Adding to the hash is not an atomic operation so you could trash the hash even when using different keys.

Make sense?

@bblimke
Copy link
Owner

bblimke commented Aug 16, 2013

I see. Ok that makes sense, cheers. It would be nice to make it at least in part http client specific. Thread.current[:webmock_responses] is very global. It could be as well used by other adapters.

@tbeauvais
Copy link
Author

ok, updated...

bblimke added a commit that referenced this pull request Aug 17, 2013
Change httpclient adapter to use thread local variables for thread safety
@bblimke bblimke merged commit 04d5fe1 into bblimke:master Aug 17, 2013
@bblimke
Copy link
Owner

bblimke commented Aug 17, 2013

I think I have merged it too early. It seems to break http client support in ruby 1.8.7

@tbeauvais
Copy link
Author

Ruby 1.8.7, what's that :-) seriously isn't that deprecated? I guess if you really want to still support 1.8.7 you can back out my changes, then when I get a chance I can look into it.

@bblimke
Copy link
Owner

bblimke commented Aug 18, 2013

whether I like it or not 1.8.7 is still supported by WebMock 1.x :)

bblimke added a commit that referenced this pull request Aug 21, 2013
@bblimke
Copy link
Owner

bblimke commented Aug 21, 2013

I reverted it. It breaks specs in async mode on Ruby 1.8.7. No idea why yet.

bblimke added a commit that referenced this pull request Feb 21, 2024
@bblimke
Copy link
Owner

bblimke commented Feb 21, 2024

@tbeauvais Just FYI I have re-introduced your excellent solution to the thread safety problem of the httpclient adapter. Thank you! Ruby 1.8.7 is not an issue anymore ;)

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