Lift ssl-context coercion to connection-pool fn
#746
Conversation
Both testing contexts are failing. The serial one is to demonstrate that the InputStream cannot be read twice without resetting, which obviously is not done by Netty/Aleph. This is also the case in the concurrent context, which was intended to resemble the original report in clj-commons#728 and is a more likely scenario, since it doesn't disable keep-alive. IIUC, the concurrent scenario could fail in an even more unpleasant way, if the test certificate file was greater than the 8192-byte buffer used to read it, but ours is not (the fix would be the same). NB: `with-http-ssl-servers` already runs things twice, so `repeatedly` is not required to make it fail, but that would be harder to read and wouldn't cover (at some level, at least) both servers.
As suggested by @DerGuteMoritz in clj-commons#728. This fixes the issue and makes the test added in the previous commit pass. Keeping the `client-ssl-context` call in `http-connection` as is, even though it might seem superfluous considering the code path taken in the test, but `http-connection` is a public API, so we have to keep the call (which for us is a no-op, if we ignore the repeated ALPN check) even for our case when the protocol is https and `ssl-context` is supplied. NOTE: This highlights a difference we are introducing here. Previously, if we specified ssl-context, but the protocol wasn't https, we would just ignore the ssl-context. Currently, we are coercing it ahead-of-time, before knowing the request protocol. This could be alleviated by wrapping the coercion in a `delay`, so it wouldn't happen until needed. Yet, given how unlikely this scenario seems, I have doubts whether it'd be worth it. I slightly dislike the repetition of `[:http1]` default value, but since it serves as documentation in `http-connection`, I decided to keep it as is rather than to extract it out. Also, I slightly dislike the repetition of a pattern to call `ensure-consistent-alpn-config` and then `coerce-ssl-client-context` but it's only now in 2 places, which I think is a better alternative than adding yet another ssl-coercion layer/wrapping function. Obviously, we cannot just move `ensure-consistent-alpn-config` to `ssl-client-context`, since ALPN is only for HTTP.
PawelStroinski
requested review from
arnaudgeiser,
KingMob and
DerGuteMoritz
as code owners
|
Hey @PawelStroinski, sorry for the delay! Responding to the main commit message:
Hmm I wonder how public that API is really - note how the
I agree. If anyone runs into it, they can easily fix it since such a combination of options doesn't make sense to begin with 😅
See above - we can get rid of this then, too.
Technically, ALPN can also be used for other purposes but in the context of Aleph's out-of-the-box support, this is true 😄 But as per above, we can get rid of this then, too. Thanks for your thoughtful notes as always! 🙏 |
Addresses #728.