Skip to content

Fix stream accounting bug when stream close leads to connection close #1603

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 2 commits into from
May 26, 2023

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented May 26, 2023

Motivation:

The connection pool manager manages a pool of connections per event-loop. It spreads load across these pools by tracking how many streams a pool has capacity for and how many streams are in use. To facilitate this each pool reports back to the pool manager when streams have been reserved and when they have been returned.

If connections are closed unexpectedly (due to an error, for example) then the pool reports this in bulk. However when the streams are closed they are also reported back to the pool manager. This means the manager can end up thinking a pool has a negative number of reserved streams which results in an assertion failure.

Modifications:

  • Check if the connection a stream is being returned to is available before reporting stream closures to the pool manager.

Result:

Motivation:

The connection pool manager manages a pool of connections per
event-loop. It spreads load across these pools by tracking how many
streams a pool has capacity for and how many streams are in use. To
facilitate this each pool reports back to the pool manager when streams
have been reserved and when they have been returned.

If connections are closed unexpectedly (due to an error, for example)
then the pool reports this in bulk. However when the streams are closed
they are also reported back to the pool manager. This means the manager
can end up thinking a pool has a negative number of reserved streams
which results in an assertion failure.

Modifications:

- Check if the connection a stream is being returned to is available
  before reporting stream closures to the pool manager.

Result:

- Better stream accounting.
- Resolved grpc#1598
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label May 26, 2023
@Lukasa Lukasa merged commit c7d6521 into grpc:main May 26, 2023
@glbrntt glbrntt deleted the gb-stream-accounting branch August 21, 2023 12:29
WendellXY pushed a commit to sundayfun/grpc-swift that referenced this pull request Aug 24, 2023
…grpc#1603)

Motivation:

The connection pool manager manages a pool of connections per
event-loop. It spreads load across these pools by tracking how many
streams a pool has capacity for and how many streams are in use. To
facilitate this each pool reports back to the pool manager when streams
have been reserved and when they have been returned.

If connections are closed unexpectedly (due to an error, for example)
then the pool reports this in bulk. However when the streams are closed
they are also reported back to the pool manager. This means the manager
can end up thinking a pool has a negative number of reserved streams
which results in an assertion failure.

Modifications:

- Check if the connection a stream is being returned to is available
  before reporting stream closures to the pool manager.

Result:

- Better stream accounting.
- Resolved grpc#1598
pinlin168 pushed a commit to sundayfun/grpc-swift that referenced this pull request Aug 24, 2023
…grpc#1603)

Motivation:

The connection pool manager manages a pool of connections per
event-loop. It spreads load across these pools by tracking how many
streams a pool has capacity for and how many streams are in use. To
facilitate this each pool reports back to the pool manager when streams
have been reserved and when they have been returned.

If connections are closed unexpectedly (due to an error, for example)
then the pool reports this in bulk. However when the streams are closed
they are also reported back to the pool manager. This means the manager
can end up thinking a pool has a negative number of reserved streams
which results in an assertion failure.

Modifications:

- Check if the connection a stream is being returned to is available
  before reporting stream closures to the pool manager.

Result:

- Better stream accounting.
- Resolved grpc#1598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion Failure in 1.16.0 for code that worked in 1.15.0
2 participants