Don't reuse Net::HTTP objects in HTTPTransport

[st0012]

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

[codecov-commenter]

Codecov Report

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

@@            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]

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

[sl0thentr0py]

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]

@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]

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.