-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
Don't reuse Net::HTTP objects in HTTPTransport
#1696
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1696 +/- ##
==========================================
- Coverage 98.57% 98.56% -0.02%
==========================================
Files 136 136
Lines 7704 7714 +10
==========================================
+ Hits 7594 7603 +9
- Misses 110 111 +1
Continue to review full report at Codecov.
|
HTTPTransport
HTTPTransport
0fa15d1
to
fd0fbd8
Compare
@sl0thentr0py this is kind of an urgent issue so I'll merge and prepare the release first. And hopefully this can be released ASAP. |
fd0fbd8
to
e40f439
Compare
hmm ok, let's release this because it's a bug but some comments for later. How does this affect performance/overhead? I assume reusing the connection if possible does have some efficiency over a fresh connection every time, but I'm not sure about the exact details. Another discrepancy with python also appears here. |
@sl0thentr0py sorry that I should have mentioned this clearer: Here's the stacktrace that should help you understand how that's done:
When I implemented it, there wasn't any guidance around how/where to build the worker and that's why it's done this way. I'm not against moving it but I won't give it a high priority either. Can you open an issue for that then we can move the discussion there? thx |
ah ok thx, ignore then!
yes absolutely! And it works absolutely fine. :) |
* master: Don't reuse Net::HTTP objects in `HTTPTransport` (getsentry#1696) Rails 7 example (getsentry#1694)
When investigating #1695, I found that
Net::HTTP
object is not supposed to be used asFaraday::Connection
that can be held and shared. Because it'll cause errorHTTP session already opened
when multiple threads try to send events with the same object.I also assume #1695 is caused by a race condition between threads that attempt to update the same
OpenSSL::SSL::SSLSocket
instance's session around here