Allow HTTP::Client to work with any IO

[waj]

I know there was attempts to make HTTP::Client more generic. In the long term that would be desirable to support connection pools or socket factories. But this change was so easy and allow other scenarios currently unsupported. For example, talk through UNIX socket. The added initializer looks ugly because still needs to fill host and port but those are used for the Host header anyway. Another option is just make the @socket available through a property so it can be externally set. The issue I found is that if close is called the next request will try to open a TCPSocket.

Dunno, again this feel incomplete but it's a simple change that open possibilities.

[jhass]

The issue I found is that if close is called the next request will try to open a TCPSocket.

Wait what? That sounds like a big foot gun right there. I think calling close should make the HTTP::Client instance unusable, regardless of this change.

Tests? :)

[asterite]

Also documentation of this method, and maybe an embedded example 🙇

[straight-shoota]

I had actually thought about this some time agoe. But it couldn't work without #9210.
It makes sense to use an IO directly and I suppose this could also continue when #6011 is resolved. Although most use cases should probably be served by that.

As @jhass mentioned, the #close behaviour needs to be fixed. IMO it would probably be best to close the client completely and not be able to reconnect. That would be a breaking change, though. So maybe for now we could just set a flag on the new initializer which disables automatic socket creation.

[asterite]

Why can't we make breaking changes when we release 1.0.0? In my mind that release sounds like the biggest breaking change ever so that later there are no breaking changes.

[waj]

We can make breaking changes, but those might not have good time of deprecation notice. I wouldn't avoid doing it anyway if we think it's worth it.

[jhass]

I think this wouldn't be a huge breaking change, as being able to reuse a closeed HTTP::Client feels like it ought to be against expectations anyways.

[waj]

The socket is re-created because the api can be used like this:

HTTP::Client.new(...) do |client|
  client.get ...
  client.post ...
end

Within the block the connection might be the same or not, depending on wether the server/protocol supports the keep alive. Or the connection might be a different one, if a connection pool were implemented.

Then the close method is available, so the same behaviour can be obtained when the block syntax can't be used. Maybe we can differentiate an internal close or just set an internal flag to allow re-creation of the connection. That would work as @straight-shoota suggest. And also I don't think it's a big breaking change except for those explicitly calling close but still reusing the client instance.

[waj]

I just implemented what @straight-shoota suggested, disabling autoreconnection only when initialized with IO, so this shouldn't be a breaking change. Also added specs and docs. We can still change how close works if we want, but I would do it in a separate PR. Note that we currently even have that behaviour documented:

crystal/src/http/client.cr

Lines 747 to 748 in 476486e

	# Closes this client. If used again, a new connection will be opened.
	def close

Should we change the name of the instance var? @socket -> @io

[jhass]

This looks good then now. I think the name of the instance var is fine, we can the noise of renaming it to a bigger refactor.

I still would prefer close to set @reconnect = false ASAP. Despite being documented right now otherwise, I find that behavior still counter intuitive. We could offer a connect kind of API to set/create the socket, keeping @reconnect to false for calls like connect(some_io) but resetting it to true for calls like connect(some_host), sort of mirroring the HTTP::Server#bind API. But totally out of scope here.

[straight-shoota]

We can still change how close works if we want, but I would do it in a separate PR.

Na, I wouldn't touch that until the entire HTTP::Client implementation gets revisited.

It's on my to-do-after-1.0 list. But the list is actually quite long. 😆

[straight-shoota]

Ideally, when using with an IO the client should not set a default host header unless explicitly asked told. The current default localhost:80 is just an arbitrary value and may cause issues if the server tries to read something from that.
I guess it's probably fine for now, though. Changing that would require a lot more refactoring.

[waj]

The Host header is mandatory in HTTP/1.1 and the client defaults to that version on every request. It's normally used only for virtual hosts so at most I think it would trigger a 404.

If you have ideas for the HTTP::Client post 1.0 I suggest we review them now, at least to tune the API to avoid breaking changes after the release. I'd love to see a more flexible HTTP client built-in in the std, but I don't have specific plans in mind.

[waj]

Correction: according to the RFC, seems like the Host header is indeed mandatory, but it must be empty in those cases: https://tools.ietf.org/html/rfc2616#section-14.23. Setting @host = "" might work for now. I'll add that to this PR.

[straight-shoota]

Yes, the host header is mandatory, but it should be empty if there's no meaningful value.

My ideas about HTTP::Client are basically expressed in #6011 and related discussions. There's also a PoC implementation in
#6001 and judging by that I'm relatively confident that we can change and enhance the implementation without breaking the public API, at least with temporary re-implementations to keep backwards compatibility.

[asterite]

I think another thing that should happen is to have HTTP::Client.get and others use an internal pool of connections. So in general people shouldn't use HTTP::Client.new at all. (similar to Go, I guess)

[straight-shoota]

@asterite Yes, that's actually one of the main features described in #6011

[waj]

Well.. finally I renamed the ivar 🙈

[asterite]

Just a note that the old annotation, @socket : TCPSocket | OpenSSL::SSL::Socket | Nil, actually was already @socket : IO | Nil because TCPSocket and OpenSSL::SSL::Socket both inherit from IO so that was the actual type seen by the compiler.

I'd still like #9052 to be considered because it would fix that problem (the ivar would actually be that union type, never go to the parent type).