Skip to content

Avoid a double-idle in the idle handler. #875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jul 6, 2020

Motivation:

The idle connectivity state can be achieved in two ways:

  1. We have no active streams and receive a GOAWAY frame, and
  2. We have no active streams and the idle timeout elapses.

The only valid state we can be in to transition to idle is ready (i.e. we have
an active channel and have received the first SETTINGS frame). We don't
currently protect against going idle twice: that is, receiving a GOAWAY
and subequently having the timeout fire. This leads to an invalid state
transition (idle to idle).

Modifications:

  • Check our 'readiness' state in the idle handler before calling
    'idle()' on the connection manager
  • Add a test.
  • Alseo cancel the timeout when the handler is removed.

Result:

We avoid invalid state transitions when double-idling.

Motivation:

The idle connectivity state can be achieved in two ways:
1. We have no active streams and receive a GOAWAY frame, and
2. We have no active streams and the idle timeout elapses.

The only valid state we can be in to transition to idle is ready (i.e. we have
an active channel and have received the first SETTINGS frame). We don't
currently protect against going idle twice: that is, receiving a GOAWAY
and subequently having the timeout fire. This leads to an invalid state
transition (idle to idle).

Modifications:

- Check our 'readiness' state in the idle handler before calling
  'idle()' on the connection manager
- Add a test.
- Alseo cancel the timeout when the handler is removed.

Result:

We avoid invalid state transitions when double-idling.
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Jul 6, 2020
@glbrntt glbrntt requested a review from Lukasa July 6, 2020 10:01
@glbrntt glbrntt added the kind/bug Feature doesn't work as expected. label Jul 6, 2020
@glbrntt glbrntt mentioned this pull request Jul 6, 2020
@Lukasa Lukasa merged commit 345438e into grpc:master Jul 6, 2020
@glbrntt glbrntt linked an issue Jul 7, 2020 that may be closed by this pull request
@glbrntt glbrntt deleted the gb-conn-manager-fix branch August 5, 2020 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected. 🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectionManager crash
2 participants