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

Refactor: Connection Pooling #1024

Open
1 of 7 tasks
technoweenie opened this issue Sep 20, 2019 · 3 comments
Open
1 of 7 tasks

Refactor: Connection Pooling #1024

technoweenie opened this issue Sep 20, 2019 · 3 comments
Projects

Comments

@technoweenie
Copy link
Member

technoweenie commented Sep 20, 2019

This is a tracking issue for a larger effort to refactor the adapters to support connection pools. #1006 proved it could work in theory, but I think the code bass will need some more massaging to fully support it. I outlined why in #1006 (comment).

TODO

  • Add #build_connection to every adapter, which outputs the fully-configured http client from options. Cleanup adapter connections #1023
  • Send ConnectionOptions to Env as :connection key.
  • Copy settings that configure the HTTP connection from RequestOptions to ConnectionOptions: :proxy, :bind, :timeout, :read_timeout, :open_timeout, :write_timeout
  • Teach Adapter#build_connection to merge ConnectionOptions with RequestOptions.
  • Remove manual merging of :proxy into RequestOptions
  • Teach Adapter#initialize to save the connection if it can:
    • If any of the moved RequestOptions settings are used, skip this!
    • If the adapter supports pooling, @pool = ConnectionPool.new(opts, &method(:build_connection))
    • If the adapter does not support pooling, @conn = build_connection
  • Add connection(&block) to yield a connection for the adapter to use:
    • If any of the moved RequestOptions settings are used, yield #build_connection)
    • If the adapter supports pooling, pass block to @pool.with
    • If the adapter does not support pooling, yield @conn

Proxy options???

# Resets temp_proxy
@temp_proxy = proxy_for_request(url)
request = build_request(method) do |req|
req.options = req.options.merge(proxy: @temp_proxy)

@technoweenie technoweenie added this to Backlog in v1.0 via automation Sep 20, 2019
@technoweenie technoweenie moved this from Backlog to In progress in v1.0 Sep 20, 2019
@iMacTia
Copy link
Member

iMacTia commented Sep 20, 2019

@technoweenie This totally looks like a breaking change. Is it OK to schedule it for Faraday 1.0?

@technoweenie
Copy link
Member Author

It's not meant to be a breaking change. I modified step 4 from:

-Move settings that configure the HTTP connection...
+Copy settings that configure the HTTP connection...

Steps 5 and 6 will check to see if any of the connection-level properties are set on the request, and if so will generate a brand new connection object (current behavior).

That said, I do think this is a fairly risky change. I also think it might be necessary... While looking at net_http_persistent and httpclient closer, I wonder if we can get 2 requests from the same Faraday::Connection to use the wrong proxy or timeout setting since they're modifying a shared connection object.

@iMacTia
Copy link
Member

iMacTia commented Sep 20, 2019

And I assume the request options would take priority over the connection ones.
That might solve the problem I've been having in the past where the adapter client configuration needs to be postponed to the very last since Faraday allows to:

  • customise connection attribute per-request (proxy, ssl, timeouts, ...)
  • call an absolute url completely different from the one provided at initialisation

Being a bit more strict on the connection initialisation and usage would ease some of the configuration issues, but would of course make it less backwards-compatible.

I appreciate the take on this though, it was definitely in my 2.0 list 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v1.0
In progress
Development

No branches or pull requests

2 participants