Fix synchronization of semaphore in retry delegating handler #3161
Conversation
abhipsaMisra
requested review from
drwill-ms,
timtay-microsoft,
andyk-ms,
brycewang-microsoft,
tmahmood-microsoft and
patilsnr
as code owners
| @@ -60,14 +73,17 @@ public override async Task SendTelemetryAsync(TelemetryMessage message, Cancella | |||
|
|
|||
| try | |||
| { | |||
| await VerifyIsOpenAsync(cancellationToken).ConfigureAwait(false); | |||
There was a problem hiding this comment.
I've added a pre-check operation to each API call - VerifyIsOpenAsync() once outside the retry block.
If the verification that the client is open is always linked with the retry block, then closing a client and then calling SendTelemetryAsync() will result in the SDK complaining that the linked cancellation token source (the one that signals in-progress operations for closure) has been canceled already, instead of complaining that the client is closed.
There was a problem hiding this comment.
Can be modified further in the "separate-delegating-handlers" PR.
| { | ||
| case ClientTransportStatus.Open: | ||
| case ClientTransportStatus.Opening: | ||
| return; |
There was a problem hiding this comment.
If the client is currently open or opening, simply exit.
| { | ||
| _isOpen = true; | ||
| case ClientTransportStatus.Closing: | ||
| throw new InvalidOperationException($"The client is currently closing. Wait until {nameof(CloseAsync)} completes and then invoke {nameof(OpenAsync)} again."); |
There was a problem hiding this comment.
If Openasync() is called while the client is closing, throw an InvalidOperationException.
There was a problem hiding this comment.
Is this documented on the client's OpenAsync call?
| { | ||
| _cancelPendingOperationsCts?.Cancel(); | ||
| _handleDisconnectCts?.Cancel(); | ||
| await base.CloseAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
@drwill-ms We had discussed encapsulating the CloseAync() within a try-catch block so that close failures are suppressed. However, as I was testing this flow I had another thought. Since we do allow open-close-open flow, I wanted to avoid suppressing any exceptions in here as failure to cleanly close the transport layer might result in issues down the line if subsequently OpenAsync() is called. For this reason, I wanted to leave this as-is and evaluate this in the future if we get feedback from customers on this flow.
| { | ||
| throw new InvalidOperationException($"The client connection must be opened before operations can begin. Call '{nameof(OpenAsync)}' and try again."); | ||
| if (GetClientTransportStatus() != ClientTransportStatus.Open) |
There was a problem hiding this comment.
This will result in operations invoked when the client is "opening" to also get rejected. It seemed like a reasonable expectation since we do want the client behavior to be deterministic.
| } | ||
|
|
||
| // Send the request for transport close notification. | ||
| _transportClosedTask = HandleDisconnectAsync(); |
There was a problem hiding this comment.
I'm worried about the lifetime of this handle disconnect code. It is called by someone else the first time and then by itself the second time.
Is any given instance ever pointing to the wrong token that it captures off of _handleDisconnectCts? Are we sure none of these calls duplicate over time?
I'd suggest diagraming it to be sure, and honestly this pattern is probably too fragile that anyone who touches this code and doesn't thoroughly understand this lifetime could break it.
I know you didn't introduce it in this PR, but perhaps we should look for a simpler impl like a callback.
There was a problem hiding this comment.
It occurs to me that this file which is supposed to be about retrying, and it looks like it does a lot more than that, especially with the addition of reconnection and overall connection (sort of) management.
Should checking the connection, reconnection attempts, etc. be a separate delegating handler, so this class doesn't get quite so full of mixed concerns?
|
Closing in favor of #3251 |
Changes from #3135, #3149
The sepmaphores associated with enabling and disabling subscriptions for callbacks should not be dependent on the Semaphore used for opening or closing the client.
The only purpose of the enable-disable events semaphore is to ensure that requests to enable and disable the subscription invoked by parallel threads are executed in a thread-safe manner.