[CRE][Limits] Handle limit flip to zero and back correctly#1762
Conversation
|
👋 bolekk, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
✅ API Diff Results - No breaking changes |
bolekk
force-pushed
the
CRE-1646-zero-queue-bug
branch
from
7fc4b52 to
7f0bfd9
Compare
| } | ||
|
|
||
| func (l *resourcePoolLimiter[N]) setOnLimitUpdate(fn func(ctx context.Context)) { | ||
| l.updater.onLimitUpdate = fn |
There was a problem hiding this comment.
does this need a write lock?
There was a problem hiding this comment.
No, it's internal and only used by constructors.
| ctx := t.Context() | ||
|
|
||
| // Consume the single available resource to force the next waiter to enqueue. | ||
| freeFirst, err := limiter.Wait(ctx, 1) |
There was a problem hiding this comment.
is there still a corner case here?:
we set the limit to zero
the node restarts
this wait blocks on ctx, which in core node is forever atm
we set the limit to 1
whatever else was waiting starts
the original is stuck forever
There was a problem hiding this comment.
Waiters don't only block on context, they block on both their context AND the queue channel. In this case it will get its channel unblocked when the limit is set to 1.
I don't believe there's a problem but if you disagree can you try proving it in a unit test (tell an LLM to do it)?
There was a problem hiding this comment.
as long as the Wait is happening in a different go routine than the set we are good; i assume that must be true in the core node. the test as written
~
Wait(ctx, foo)
Set(foo)
will block until ctx expiration in wait and never get to Set, but iiuc, that is simply test artifact
3 participants
No description provided.