Fix for b/74749605: Cancel pending backoff operations when closing streams. #564
Conversation
| @@ -316,6 +316,9 @@ export abstract class PersistentStream< | |||
|
|
|||
| this.cancelIdleCheck(); | |||
|
|
|||
| // Ensure we don't leave a pending backoff operation queued. | |||
There was a problem hiding this comment.
This comment can move above the idle cancel and cover both:
// Ensure we don't leave pending timer-based operations queued.
There was a problem hiding this comment.
Hrm. I had trouble creating a unified comment that I didn't feel was overly general or misleading, so instead I just commented both of them separately.
FWIW- the idle timer will only be active if the stream is currently connected when close() is called, and the backoff timer will only be active if the stream is not currently connected when close() is called.
| @@ -316,6 +316,9 @@ export abstract class PersistentStream< | |||
|
|
|||
| this.cancelIdleCheck(); | |||
|
|
|||
| // Ensure we don't leave a pending backoff operation queued. | |||
| this.backoff.cancel(); | |||
There was a problem hiding this comment.
It's a little weird that cancelling the idle and backoff timers look different. Is there any way to make these uniform? I suppose this happens because the backoff timer is held within the backoff object but the idle timer is held directly?
I think this may be non-actionable, but mentioning it anyway.
| // Stream can be stopped while waiting for backoff to complete. | ||
| return; | ||
| } | ||
| assert( |
There was a problem hiding this comment.
Man am I gun-shy about this assertion. I'm somewhat in favor of just leaving the old behavior in place to keep this consistent with the other callbacks.
There was a problem hiding this comment.
Yeah... I am torn between being as strict as possible with our state transitions vs being defensive. I mostly changed it to an assert, since as-is it's basically dead code.
I put it back though and just tweaked the comment.
| @@ -316,6 +316,9 @@ export abstract class PersistentStream< | |||
|
|
|||
| this.cancelIdleCheck(); | |||
|
|
|||
| // Ensure we don't leave a pending backoff operation queued. | |||
There was a problem hiding this comment.
Hrm. I had trouble creating a unified comment that I didn't feel was overly general or misleading, so instead I just commented both of them separately.
FWIW- the idle timer will only be active if the stream is currently connected when close() is called, and the backoff timer will only be active if the stream is not currently connected when close() is called.
| @@ -316,6 +316,9 @@ export abstract class PersistentStream< | |||
|
|
|||
| this.cancelIdleCheck(); | |||
|
|
|||
| // Ensure we don't leave a pending backoff operation queued. | |||
| this.backoff.cancel(); | |||
| // Stream can be stopped while waiting for backoff to complete. | ||
| return; | ||
| } | ||
| assert( |
There was a problem hiding this comment.
Yeah... I am torn between being as strict as possible with our state transitions vs being defensive. I mostly changed it to an assert, since as-is it's basically dead code.
I put it back though and just tweaked the comment.
No description provided.