socketry / async-http

MIT License
298 stars 45 forks source link

Allow using custom endpoints for Async::HTTP::Internet #51

Closed binarycode closed 1 week ago

binarycode commented 4 years ago

This PR introduces changes that allow passing custom Async::HTTP::Endpoint to Async::HTTP::Internet. This could be useful for example when we need to provide custom SSL options to requests.

ioquatix commented 4 years ago

I agree with your assessment, but expanding the interface in this direction I need a little bit of time to think about it.

binarycode commented 4 years ago

The thing is I want to add support of SSL options (https://github.com/lostisland/faraday/blob/master/lib/faraday/options/ssl_options.rb) to async-http-faraday adapter. I can see 2 approaches to that:

  1. Rewrite the adapter to use lower-level Async::HTTP::Client that allows customizing endpoints. The downside of this approach is that I'll have to basically duplicate the logic of Async::HTTP::Internet in the adapter to allow for persistent connections.
  2. Modify Async::HTTP::Internet to allow passing customized endpoints - that's why I created this PR.

What approach do you think is better? Should I try going with the first one to avoid changing Async::HTTP::Internet API?

ioquatix commented 4 years ago

Can you make a subclass and add the appropriate hooks so you can configure the SSL to your requirements in the faraday adapter?

binarycode commented 4 years ago

I guess that would be almost the same as the first approach, as most of the Async::HTTP::Internet logic is contained in the #call method that I'll need to rewrite to allow endpoint customization. Plus one of the downsides is that any upstream changes in Async::HTTP::Internet could possibly break this subclass and that will not be detected by CI, as the adapter is not pinned to a particular async-http version. Anyway I'll try this approach and make a PR to the adapter gem today.

ioquatix commented 4 years ago

Can we expose the right internal hooks for Async::HTTP::Internet and add specs for them?

Do you imagine all clients would use the same SSL settings? I imagine something like:

internet = MyInternet.new

internet.client_options[hostname] = ssl_options

Or something like that (those names/methods had all of 30 seconds of brain power).

When you have specific SSL state, does it impact all connections or just some specific ones?

binarycode commented 4 years ago

Well if we are talking about the faraday use-case, SSL can be configured only per Faraday::Connection, so all requests made through the adapter wrapped by that connection will use the same SSL settings.

One way this could be configured in Async::HTTP::Internet is to add a new initialize option:

def initialize(**options)
  @clients = {}
  @ssl_context = options.delete(:ssl_context)
  @options = options
end

or a separate method:

attr_writer :ssl_context

And then use that instance variable to build the endpoint:

def call(method, url, headers = [], body = nil)
  endpoint = Endpoint.parse(url, nil, ssl_context: @ssl_context)

Implementing the SSL options per-host will be trickier, as the actual per-host Async::HTTP::Client caching done by the Async::HTTP::Internet is an internal implementation detail.

softbrada commented 1 year ago

any news on this PR? i was trying to set SSL options today also and found this PR

ioquatix commented 1 week ago

Sorry this took a while to merge! Made a few minor changes and it's good to go.