Skip to content

Commit 4414396

Browse files
Lift ssl-context coercion to connection-pool fn
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.
1 parent acc7c47 commit 4414396

File tree

1 file changed

+11
-2
lines changed

1 file changed

+11
-2
lines changed

src/aleph/http.clj

+11-2
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,10 @@
229229
(when (and force-h2c? (not-any? #{:http2} http-versions))
230230
(throw (IllegalArgumentException. "force-h2c? may only be true when HTTP/2 is enabled."))))
231231

232-
(let [log-activity (:log-activity connection-options)
232+
(let [{:keys [log-activity
233+
ssl-context
234+
http-versions]
235+
:or {http-versions [:http1]}} connection-options
233236
dns-options' (if-not (and (some? dns-options)
234237
(not (or (contains? dns-options :transport)
235238
(contains? dns-options :epoll?))))
@@ -242,7 +245,13 @@
242245
(assoc :name-resolver (netty/dns-resolver-group dns-options'))
243246

244247
(some? log-activity)
245-
(assoc :log-activity (netty/activity-logger "aleph-client" log-activity)))
248+
(assoc :log-activity (netty/activity-logger "aleph-client" log-activity))
249+
250+
(some? ssl-context)
251+
(update :ssl-context
252+
#(-> %
253+
(common/ensure-consistent-alpn-config http-versions)
254+
(netty/coerce-ssl-client-context))))
246255
p (promise)
247256
create-pool-fn (or pool-builder-fn
248257
flow/instrumented-pool)

0 commit comments

Comments
 (0)