Skip to content

Don't reuse Net::HTTP objects in HTTPTransport#1696

Merged
st0012 merged 2 commits intomasterfrom
dont-reuse-http-objects
Jan 23, 2022
Merged

Don't reuse Net::HTTP objects in HTTPTransport#1696
st0012 merged 2 commits intomasterfrom
dont-reuse-http-objects

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Jan 23, 2022 •
edited
Loading

When investigating #1695, I found that Net::HTTP object is not supposed to be used as Faraday::Connection that can be held and shared. Because it'll cause error HTTP 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

@st0012 st0012 self-assigned this Jan 23, 2022
@st0012 st0012 added this to the 5.0.1 milestone Jan 23, 2022
@st0012 st0012 requested a review from sl0thentr0py January 23, 2022 16:01
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 23, 2022 •
edited
Loading

Codecov Report

Merging #1696 (fd0fbd8) into master (ee1cf06) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/transport/http_transport.rb 100.00% <100.00%> (ø)
...-ruby/spec/sentry/transport/http_transport_spec.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/background_worker.rb 94.59% <0.00%> (-5.41%) ⬇️
sentry-ruby/lib/sentry/breadcrumb.rb 100.00% <0.00%> (+3.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee1cf06...fd0fbd8. Read the comment docs.

@st0012 st0012 changed the title Don't reused Net::HTTP objects in HTTPTransport Don't reuse Net::HTTP objects in HTTPTransport Jan 23, 2022
@st0012 st0012 force-pushed the dont-reuse-http-objects branch from 0fa15d1 to fd0fbd8 Compare January 23, 2022 16:20
@st0012 st0012 mentioned this pull request Jan 23, 2022
Closed
@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Jan 23, 2022

@sl0thentr0py this is kind of an urgent issue so I'll merge and prepare the release first. And hopefully this can be released ASAP.

@st0012 st0012 force-pushed the dont-reuse-http-objects branch from fd0fbd8 to e40f439 Compare January 23, 2022 16:31
@st0012 st0012 merged commit c2f9e06 into master Jan 23, 2022
@st0012 st0012 deleted the dont-reuse-http-objects branch January 23, 2022 16:37
@sl0thentr0py
Copy link
Copy Markdown
Member

sl0thentr0py commented Jan 23, 2022

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.
In ruby, we have the BackgroundWorker instance on the top level Sentry module while in python, we have the worker on the transport itself. If possible, I would like to resolve these inconsistencies in architecture longer term so we have the same high level logic across SDKs.

@st0012
Copy link
Copy Markdown
Contributor Author

st0012 commented Jan 23, 2022

@sl0thentr0py sorry that I should have mentioned this clearer: faraday also initializes new instances for every request. So it's the removal of faraday that changed the behavior. This fix just reverts it.

Here's the stacktrace that should help you understand how that's done:

/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-1.9.3/lib/faraday/adapter.rb:47:in `connection'
/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-net_http-1.0.1/lib/faraday/adapter/net_http.rb:66:in `call'
/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-1.9.3/lib/faraday/middleware.rb:18:in `call'
/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-1.9.3/lib/faraday/rack_builder.rb:154:in `build_response'
/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-1.9.3/lib/faraday/connection.rb:516:in `run_request'
/Users/st0012/.rbenv/versions/2.7.5/lib/ruby/gems/2.7.0/gems/faraday-1.9.3/lib/faraday/connection.rb:281:in `post'
/Users/st0012/projects/sentry-ruby/sentry-ruby/lib/sentry/transport/http_transport.rb:33:in `send_data'

In ruby, we have the BackgroundWorker instance on the top level Sentry module while in python, we have the worker on the transport itself. If possible, I would like to resolve these inconsistencies in architecture longer term so we have the same high level logic across SDKs.

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

@sl0thentr0py
Copy link
Copy Markdown
Member

sl0thentr0py commented Jan 23, 2022

faraday also initializes new instances for every request.

ah ok thx, ignore then!

there wasn't any guidance around

yes absolutely! And it works absolutely fine. :)
Moving further discussion outside to not pollute this issue more.

lewispb added a commit to lewispb/sentry-ruby that referenced this pull request Jan 23, 2022
@lewispb
Merge branch 'master' into lb/redis
4505216 
* master:
  Don't reuse Net::HTTP objects in `HTTPTransport` (getsentry#1696)
  Rails 7 example (getsentry#1694)
@st0012 st0012 mentioned this pull request Jan 24, 2022
Closed
3 tasks
@rreckonerr rreckonerr mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sl0thentr0py sl0thentr0py Awaiting requested review from sl0thentr0py

Assignees

@st0012 st0012

Labels

sentry-ruby

Projects

None yet

Milestone

5.0.1

Development

Successfully merging this pull request may close these issues.

3 participants

@st0012 @codecov-commenter @sl0thentr0py