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

Fix test adapter thread safety #1380

Merged
merged 1 commit into from Jan 15, 2022
Merged

Conversation

iMacTia
Copy link
Member

@iMacTia iMacTia commented Jan 13, 2022

Description

Fix test adapter thread safety.
This is a follow-up attempt from #1379
Fixes #1365

Additional Notes

Tested locally using "Steps to reproduce" from #1365.
Below are the benchmark results:

# With sleep
            user     system      total        real
Before: 3.547856   0.051316   3.599172 ( 13.638507)
After:  4.511469   0.051330   4.562799 ( 14.601563)

# Without sleep
            user     system      total        real
Before: 2.324904   0.034064   2.358968 (  2.357287)
After:  2.897183   0.030553   2.927736 (  2.925748)

The change does, as expected, slow down the test adapter, but this doesn't seem to be too much (~ 24%).
Considering this is mostly used in test environments rather than production ones, it should be safe to have a small dip in performance. It's also worth noting that this only applies to a multi-threaded environment.
Most applications will use the test adapter in a single thread and normally recreate the connection for each test example, so there would be no performance impact in that scenario

@iMacTia iMacTia self-assigned this Jan 13, 2022
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Easy to view the change with less significant formatting:
https://github.com/lostisland/faraday/pull/1380/files?w=1

@iMacTia
Copy link
Member Author

iMacTia commented Jan 13, 2022

Nice trick! I didn't know that 😮!

I've asked @tchutw to have a look, I 'd wait for that before merging this time 😅

@iMacTia iMacTia merged commit 577f0d3 into main Jan 15, 2022
@iMacTia iMacTia deleted the fix-test-adapter-thread-safety branch January 15, 2022 08:58
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.

Faraday::Adapter::Test::Stubs is not thread safe
2 participants