Add ConcurrencyLimit and its default implementation
#13014
Conversation
Motivation: As a preparatory step for resolving #12842, we need a construct that limits concurrency asynchronously. Modifications: - Add `ConcurrencyLimit` and `ConcurrencyLimitHandler` - Add the default implementation `SimpleConcurrencyLimit` Result: - In our future work, we can allow a user to specify a `ConcurrencyLimit` when building a `DnsNameResolver` and use it for limiting the concurrency of DNS queries in `DnsQueryContext.writeQuery()`
|
@trustin welcome back buddy :) |
There was a problem hiding this comment.
I would allow using different queues types for pendingHandlers and would save the PendingHandler in a local variable outside of the CAS loop to reuse it in case of failing CAS.
Furthermore, not a blocker for this, I would implement a more constrained version (with lighter concurrent ops) for single threaded usage too (where it makes sense, to be used always by the same event loop)
| assertEquals(expectedPermitsInUseOnAcquisition, limit.permitsInUse()); | ||
| releaser.run(); | ||
| assertEquals(expectedPermitsInUseOnAcquisition - 1, limit.permitsInUse()); | ||
| events.add("acquired"); |
There was a problem hiding this comment.
I think it'd be good if we declare class-level variables for event names.
| * | ||
| * @param releaser the {@link Runnable} that releases the permit | ||
| */ | ||
| void permitAcquired(Runnable releaser); |
There was a problem hiding this comment.
What's the idea behind relying on the callee to invoke the releaser, vs. automatically releasing a permit upon the completion of this method?
| pendingHandlers.add(pendingHandler); | ||
| return; |
There was a problem hiding this comment.
We have a data-race here where all permits could have been returned, and all releasers could have been called, before we manage to add our pending handler to the queue.
| pendingHandlers.add(pendingHandler); | ||
| return; |
There was a problem hiding this comment.
Same data-race exists here, with a smaller window.
| assert nextPermitInUse >= 0 : "Released more than acquired"; | ||
| if (permitsInUseUpdater.compareAndSet(SimpleConcurrencyLimit.this, | ||
| currentPermitInUse, nextPermitInUse)) { | ||
| final PendingHandler pendingHandler = pendingHandlers.poll(); |
There was a problem hiding this comment.
If the pending handler is non-null, we could in theory "hand over" our existing permit, and skip the decrement/increment dance.
|
I renamed |
trustin
force-pushed
the
concurrency_limit
branch
from
bd54d1b to
21794b0
Compare
|
Had a chat with @normanmaurer a few days ago and we concluded that at the moment we don't really need any pluggability for rate-limiting DNS queries - just simple rate limiting implemented directly in our resolver implementation will do. Once we find similar use cases in other places in Netty, we may resurrect this PR, but for now, let me close it. |
5 participants
Motivation:
As a preparatory step for resolving #12842, we need a construct that limits concurrency asynchronously.
Modifications:
ConcurrencyLimitandConcurrencyLimitHandlerSimpleConcurrencyLimitResult:
ConcurrencyLimitwhen building aDnsNameResolverand use it for limiting the concurrency of DNS queries inDnsQueryContext.writeQuery()