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.

Sign up for GitHub

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

Jump to bottom

NetHttpPersistent adapter reuse SSL connections #793

Merged
merged 3 commits into from
May 4, 2018
Merged

NetHttpPersistent adapter reuse SSL connections #793

merged 3 commits into from
May 4, 2018

Conversation

katsuya94
Copy link
Contributor

@katsuya94 katsuya94 commented Apr 26, 2018 •
edited
Loading

Description

Fixes #791 by only setting SSL attributes on the Net::HTTP::Persistent instance when params are changed.

@iMacTia this is a (not working yet) first stab at fixing the issue. The problem I'm running into is in order to determine when to actually update SSL params I need to compare the current and new values. Unfortunately some of these values like cert_store are of non-comparable types like OpenSSL::X509::Store. Any ideas?

Update: fixed issues with object comparison by caching the blank cert store.

iMacTia
iMacTia reviewed Apr 27, 2018
View reviewed changes
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Hey, thanks for giving this shot!

You can probably use the object_id for cert_store and other incomparable fields as ssl_options shouldn't really change between requests.

I also left a couple of comments as some things are not clear to me.

lib/faraday/adapter/net_http_persistent.rb Outdated
def cached_connection(url, proxy_uri)
(@cached_connection ||= {})[[url.scheme, url.host, url.port, proxy_uri]] ||= yield
def http_set(http, attr, value)
if http.sent(attr) != value
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be send ?

lib/faraday/adapter/net_http_persistent.rb Outdated
end

def cached_connection
@cached_connection ||= yield
Copy link
Member

Choose a reason for hiding this comment

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

Removing the hash caching, we're going to reuse the same connection even though the url changes. Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Net::HTTP::Persistent has logic for using different connections for different URI bases (including differences in proxy) internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in 2.9.4 https://github.com/drbrain/net-http-persistent/blob/v2.9.4/lib/net/http/persistent.rb#L613-L619
in 3.0.0 https://github.com/drbrain/net-http-persistent/blob/v3.0.0/lib/net/http/persistent.rb#L608-L611

katsuya94
katsuya94 commented Apr 27, 2018
View reviewed changes
lib/faraday/adapter/net_http.rb
cert_store
@cert_store = OpenSSL::X509::Store.new
@cert_store.set_default_paths
@cert_store
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found the issue. object_id comparison wasn't working because the NetHttp adapter was newing up a cert store every time. This memoizes it.

@katsuya94
Copy link
Contributor Author

I think this is working now. we'll see what CI has to say though.

@katsuya94 katsuya94 changed the title WIP NetHttpPersistent adapter reuse SSL connections NetHttpPersistent adapter reuse SSL connections Apr 27, 2018
@katsuya94
Copy link
Contributor Author

@iMacTia Any thoughts on the updates?

@iMacTia
Copy link
Member

iMacTia commented Apr 30, 2018

I’m really sorry @katsuya94, really wanted to have another look but I still haven’t got the time :(
Will do my best to review them reasonably soon as I would like to include this into the next release!

@katsuya94
Copy link
Contributor Author

No worries! Take your time.

@iMacTia
Copy link
Member

iMacTia commented May 1, 2018

@katsuya94 I had a quick look at changes and they look good to me!

The only thing I'm not completely sure about is the comeback on relying on Net::HTTP::Persistent caching of connection.
In #778 a fix was introduced that sensibly improved performances by managing the cache on our side.

I would like to re-run the same tests @grosser did on that PR to see if we're regressing on that side.
If you have some time, would you like to run it? Otherwise I'll do as soon as I can.

@katsuya94
Copy link
Contributor Author

net-http-persistent 2.9.4

faraday/ (net_http_persistent_fix|✔) % git checkout net_http_persistent_fix
faraday/ (net_http_persistent_fix|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
3.403531900999951
faraday/ (net_http_persistent_fix|✔) % git checkout origin/master
faraday/ (:4381c2d|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
4.028428414000018
faraday/ (:4381c2d|✔) % git checkout 6ddf9c3b80d2eee6673c72d82d181a810746088c
faraday/ (:6ddf9c3|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
3.9857757019999553

net-http-persistent 3.0.0

faraday/ (:6ddf9c3|✔) % git checkout net_http_persistent_fix
faraday/ (net_http_persistent_fix|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
3.5376455999999052
faraday/ (net_http_persistent_fix|✔) % git checkout origin/master
faraday/ (:4381c2d|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
5.190602235999904
faraday/ (:4381c2d|✔) % git checkout 6ddf9c3b80d2eee6673c72d82d181a810746088c
faraday/ (:6ddf9c3|✔) % ruby -Ilib -e 'require "faraday"; require "benchmark"; puts Benchmark.realtime { 20.times { Faraday.get "https://g.co/sddfsfds" } }'
3.96499722700014

6ddf9c3 was the commit before #778 was merged

@iMacTia
Copy link
Member

iMacTia commented May 4, 2018

@katsuya94 Thanks for running the tests!
I'm surprised master is actually the slowest (even slower than before the fix!).
Anyway, the important thing is that your branch yields the best results.
And that tests are all green, of course 😃

@iMacTia iMacTia merged commit 49112d2 into lostisland:master May 4, 2018
grosser
grosser reviewed May 4, 2018
View reviewed changes
lib/faraday/adapter/net_http_persistent.rb
end

def cached_connection(url, proxy_uri)
(@cached_connection ||= {})[[url.scheme, url.host, url.port, proxy_uri]] ||= yield
def cached_connection
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well inline it at that point :)

@olleolleolle olleolleolle mentioned this pull request May 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iMacTia iMacTia iMacTia left review comments

@grosser grosser grosser left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Faraday::Adapter::NetHttpPersistent does not reuse requests for SSL with Net::HTTP::Persistent < 3.0.0
3 participants
@katsuya94 @iMacTia @grosser