-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #272 #274
Fixes #272 #274
Conversation
The git comment is incorrect. |
val http: HttpExt = Http() | ||
val flow: Flow[HttpRequest, HttpResponse, Future[OutgoingConnection]] = | ||
if (request.uri.scheme == "https") | ||
http.outgoingConnectionHttps(request.uri.authority.host.address(), request.uri.effectivePort, connectionContext = connectionContext, settings = settings.connectionSettings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outgoingConnectionHttps
and outgoingConnection
calls create new connections for each request as I understand it? The current singleRequest
call benefits from connection pooling, so I'd be concerned about the additional overhead with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, there's an overhead with using the low-level connection API but it has a nice feature: the lifecycle of the connection is tied to the lifecycle of the Akka flow graph that uses it. This makes for clean shutdown behavior!
The high-level host API has the advantage of connection polling but there are two caveats to address:
-
https://doc.akka.io/docs/akka-http/current/client-side/host-level.html#pool-shutdown
Unless skuber manages its own connection pool at initialization & close, we'll get the annoying
akka.stream.AbruptTerminationException
. This would require some change to the API to allow clients to pass at initialization a factory function to create connection pool so thatclose
could shut it down. -
https://doc.akka.io/docs/akka-http/current/client-side/host-level.html#using-a-host-connection-pool
A long-running
GET
request blocks other requests. I don't know if this is a significant concern here.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For watch
commands (which use long polling GET) skuber uses a tailored pool (see https://github.com/doriordan/skuber/blob/master/client/src/main/scala/skuber/api/watch/LongPollingPool.scala) to avoid the issue you mention with respect to blocking other requests and also avoid timeouts. For other use cases the requests are generally expected to not be long-running.
The Akka docs generally recommend using singleRequest
over outgoingConnection...
- see https://doc.akka.io/docs/akka-http/current/client-side/connection-level.html
// The `outgoingConnection` API is very low-level. Use it only if you already have a `Source[HttpRequest, _]`
// (other than Source.single) available that you want to use to run requests on a single persistent HTTP
// connection.
//
// Unfortunately, this case is so uncommon, that we couldn't come up with a good example.
//
// In almost all cases it is better to use the `Http().singleRequest()` API instead.
Is the AbruptTerminationException
causing serious concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main concern is that AbruptTerminationException
makes it difficult to tell whether there is a genuine problem somewhere or not.
I would be OK in keeping the host-level client connection strategy as long as the code is updated to ensure that the connection pools are shutdown. This should then avoid these exceptions.
Thanks for mentioning the tailored pool. It's a really neat trick!
I haven't used this part of skuber yet but it seems to me that it contributes to the AbruptTerminationException
problem since the tailored pools are not currently shutdown.
Looking at this code: https://github.com/doriordan/skuber/blob/master/client/src/main/scala/skuber/api/client/package.scala#L26
The pool signature is:
type Pool[T] = Flow[(HttpRequest, T), (Try[HttpResponse], T), NotUsed]
Based on the Akka doc (https://doc.akka.io/docs/akka-http/current/client-side/host-level.html#using-a-host-connection-pool), it seems to me that it ought to be instead:
type Pool[T] = Flow[(HttpRequest, T), (Try[HttpResponse], T), HostConnectionPool]
My understanding is that at the end of the flow graph, we would get the materialized value -- i.e., the connection pool that was used -- and this would allow us to cleanly shut it down.
What about then applying the same trick for other kinds of requests?
The difference really is that for watch-related operations, there's a connection pool created per invocation of the watch APIs in Skuber whereas other operations would reuse the same connection pool. The reused connection pool could be shutdown at skuber's close.
It seems to me that this ought to solve the original problem -- i.e., no AbruptTerminationException
for any happy path -- while retaining the host-level connection benefits.
cfcc197
to
ce74dc1
Compare
@doriordan This is a complete rewrite of the PR that restores the original See To achieve this, I had to make API-breaking changes to the skuber API. |
Closing because the revised PR is for a different issue: #280 |
With the fix, the example in the issue produces the following behavior:
Clean shutdown, no warnings, no errors.