Add max wait for connection borrowing in pool manager #6387
Conversation
| pool <- mkPool( | ||
| maxTotal = 1, | ||
| maxIdleDuration = Duration.Inf, | ||
| maxBorrowDuration = 2.seconds, |
There was a problem hiding this comment.
Anecdotally says, but at local, this test-suite takes about 0.001s on execution. However, at CI it may become flaky. But still, I didn't mark it as 'flaky' before receiving any facts about its flakiness.
There was a problem hiding this comment.
Lovely. It turned out that the other test suite became flaky.
danicheg
force-pushed
the
blaze-client-pool-manager
branch
from
ad37da2
to
58a5fd6
Compare
blaze-client/src/main/scala/org/http4s/blaze/client/PoolManager.scala
Outdated
Show resolved
Hide resolved
blaze-client/src/main/scala/org/http4s/blaze/client/PoolManager.scala
Outdated
Show resolved
Hide resolved
| F.delay( | ||
| logger.warn(s"Requesting connection for $key has exceed timeout") | ||
| ) *> | ||
| decrConnection(randKey) | ||
| } *> | ||
| createConnection(key, callback) | ||
| else | ||
| F.delay( | ||
| logger.debug( | ||
| s"No connections available for the desired key, $key. Adding to waitQueue: $stats" | ||
| ) | ||
| ) *> | ||
| addToWaitQueue(key, callback) | ||
|
|
||
| case None => // we're full up. Add to waiting queue. | ||
| F.delay( | ||
| logger.debug( | ||
| s"No connections available for $key. Waiting on new connection: $stats" | ||
| ) | ||
| ) *> | ||
| addToWaitQueue(key, callback) | ||
| } | ||
|
|
||
| F.delay(logger.debug(s"Requesting connection for $key: $stats")).productR(go()).as(None) | ||
| } else | ||
| F.delay(callback(Left(new IllegalStateException("Connection pool is closed")))).as(None) | ||
| } | ||
| F.delay(callback(Left(ConnectionBorrowingException(key)))) | ||
|
|
There was a problem hiding this comment.
Is this the only reason this can get canceled? I'm trying to reason about when we'd see this message instead of the timeout on line 311, and whether the message (and callback invocation) is accurate.
There was a problem hiding this comment.
Uh-oh, you're 100% right. Thanks for pointing that out. For sure, canceling could be caused by various reasons. But do you hint that we shouldn't invoke the callback here? I feel we can't be too sure if CE will resolve the asynchronous deadlock via the standard canceling mechanism smoothly. According to timeoutTo scaladoc:
"If the source is uncancelable, the resulting effect will wait for it to complete before evaluating the fallback."
So I've tried to soften the blow.
There was a problem hiding this comment.
I'm not sure what would make that source uncancelable. And if it is, instead of never completing, won't it just ignore the borrow? It seems you're partially reinventing timeoutTo.
There was a problem hiding this comment.
After the 'cancelation in CE3' tutorial by Daniel, I'm sure I was wrong and actually was reinventing the wheel with Ref here, as you pointed out (if someone is interested, link to the discussion https://discord.com/channels/632277896739946517/632278585700384799/977634027651608626).
I think a few hot-takes should be highlighted. Also, this answers the question 'what makes the source uncancelable':
asyncis uncancelable during registration;- if one defines
asyncwithout cancel token (i. e. usingNoneinstead of actual effect), thenasyncwill be uncancellable; - things created via
delayis uncancellable too.
So summing up, I'm not sure that introducing timeout via timeoutTo will work as expected all the time. I think that for sure could be scenarios when
- the whole
asynceffect will be considered uncancelable, - and
timeoutTowill wait for the natural execution ofasyncthen produce an exception (for timeout limit exceeding case).
And I don't think that what we actually want is that behavior.
There was a problem hiding this comment.
Right. This never returns:
IO.uncancelable(_ => IO.never)
.timeoutTo(1.second, IO.unit)
.unsafeRunSync()
|
No rush, FYI this is a blocker for blaze schism :) |
|
I can recreate this in blaze if we have to. |
|
I think I'll port this one to the blaze repo. |
|
Superseded by http4s/blaze#681. |
Closes http4s/blaze#654. It's better to review with hidden whitespaces. The impl is super straightforward, and I could be missing something stupidly obvious.