fixes #773 - Wire up response-executor option in connection-pool #775
fixes #773 - Wire up response-executor option in connection-pool #775gabrielfranca95 wants to merge 2 commits intoclj-commons:masterfrom
Conversation
gabrielfranca95
requested review from
DerGuteMoritz and
arnaudgeiser
as code owners
gabrielfranca95
force-pushed
the
fix-773-response-executor
branch
from
e32ad71 to
03db885
Compare
DerGuteMoritz
left a comment
DerGuteMoritz
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution! 👍
src/aleph/http.clj
Outdated
| (defn- get-pool | ||
| "Returns the underlying IPool from a pool or PoolWithExecutor." | ||
| ^IPool [pool] | ||
| (if (instance? PoolWithExecutor pool) |
There was a problem hiding this comment.
How about introducing an IConnectionPool protocol and implement it for both ConnectionPool (aka PoolWithExecutor) and Pool instead of manually dispatching here and in get-response-executor?
There was a problem hiding this comment.
That was actually my first thought too! But your suggestion below (proxying IPool) is even cleaner. Let’s skip the extra protocol and go straight for the proxy approach. 🚀
There was a problem hiding this comment.
Cool 👍 Protocol could still make sense to get rid of the instance? check in request, though!
There was a problem hiding this comment.
Or do you feel that's overkill? FWIW, we could also just drop the (when (instance? ...) ...) as keyword lookup on any non-mappy object will return nil anyway.
There was a problem hiding this comment.
Cool! I went with your suggestion to just drop the (instance? ...) check relying on the keyword lookup is way simpler and works perfectly. Pushed the changes! 🚀
…s#773) - Add PoolWithExecutor record to associate response-executor with pool - Update connection-pool to return wrapped pool when response-executor is provided - Update request function to extract executor from pool wrapper - Add test to verify executor affinity correctly
gabrielfranca95
force-pushed
the
fix-773-response-executor
branch
from
03db885 to
7891663
Compare
2 participants
Description:
Fixes #773
Problem
The documented :response-executor option in aleph.http/connection-pool was not being used. The request function was defaulting to default-response-executor for the result deferred, effectively ignoring any executor configured at the connection pool level.
Solution
This PR wires up the :response-executor by ensuring it is accessible within the request function when a custom connection pool is used.
Changes:
Introduced a PoolWithExecutor record that wraps the underlying IPool and stores the response-executor.
Updated aleph.http/connection-pool to return this wrapper when a :response-executor is provided in the options.
Updated aleph.http/request to:
Check if the provided pool is a wrapper and extract the custom executor.
Use the pool's executor for the result deferred (falling back to the default if none is present).
Correctly extract the raw pool for flow/acquire, flow/dispose, and flow/release operations.
Added a regression test test-connection-pool-response-executor to verify that the response deferred actually uses the provided executor.
CI Fix: Updated
project.clj
to use org.clojure/clojure "1.12.0" instead of result-less 1.12.2, resolving a dependency resolution failure in the CI build.