alts: Queue ALTS handshakes once limit is reached rather than dropping. #6884
Conversation
matthewstevenson88
added
the
Type: Feature
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6884 +/- ##
==========================================
- Coverage 83.75% 83.60% -0.15%
==========================================
Files 286 286
Lines 30792 30792
==========================================
- Hits 25789 25745 -44
- Misses 3948 3981 +33
- Partials 1055 1066 +11
|
| if !clientHandshakes.TryAcquire(1) { | ||
| return nil, nil, errDropped | ||
| if err := clientHandshakes.Acquire(ctx, 1); err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
Can you please clarify how returning the actual error help queuing instead of dropping? Is there another PR after this?
There was a problem hiding this comment.
In the current code, we call TryAcquire on the semaphore. If the semaphore is capped out, then this immediately returns false, so we immediately return errDropped to the user.
When we replace the TryAcquire call with Acquire instead, we will block on trying to acquire the semaphore until ctx times out. Internally to the semaphore, there is a queue of "acquire attempts", so calling Acquire effectively adds the current goroutine to this queue.
As explained in b/312467484, we want to do this so that we have uniform behavior among the 3 languages.
Does that make sense / help clarify?
There was a problem hiding this comment.
Thanks for the clarification. This makes sense. Can you please add a comment that Acquire blocks until it can acquire? Thank you!
|
@easwars If you have time, would you also be able to take a quick pass? |
|
Thanks for taking a look!
There is an e2e test that ensures that this PR works correctly: grpc-go/credentials/alts/alts_test.go Line 340 in 33a60a8 Before this PR, that test will just get several errors, log these errors, and have to retry. With this PR, the same thing occurs but instead of returning an error and logging, we are waiting until one of the ongoing handshakes completes, then proceeding. From the user perspective, there is very little difference, but we're making this change to be aligned with the C++ and Java behavior. |
See b/312467484.
RELEASE NOTES: none