Skip to content
New issue

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

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

Already on GitHub? # to your account

Avoid leaking goroutines on close #101

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Avoid leaking goroutines on close #101

merged 2 commits into from
Jul 23, 2024

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Jun 28, 2024

Fixes #92

  • Avoids leaking timers in shutdown and Wait if the functions exit before the timeouts expire
  • Shortens the timeout for an unhandled error from 10 minutes to 1 second. It looks like the 10 minute timeout was added in calling conn close before stream close results in a soft deadlock #24 / Update close to not block on shutdown #25 without a rationale... happy to adjust this PR if there are other things aspects to consider. The paths where errors could be handled are:
    1. client calls CloseWait(). We know Close() is immediately followed by Wait(), a second seems entirely sufficient.
    2. client calls Close() followed by Wait(). We don't control the calls, but the expectation is that Close() is immediately followed by Wait(), so a second seems entirely sufficient.
    3. client calls NotifyClose(), GOAWAYFRAME notifies lastStreamChan, client responds by calling Wait(). lastStreamChan is notified before the shutdown is begun, which gives the caller a second to call Wait() if they want to handle the shutdown error. This is the only path I don't have a great intuition of, but a second to process the channel notification seems reasonable. I also can't find a single use of this function in a public github search.

liggitt added 2 commits June 28, 2024 12:53
Signed-off-by: Jordan Liggitt <liggitt@google.com>
Signed-off-by: Jordan Liggitt <liggitt@google.com>
@liggitt liggitt changed the title Close leak Avoid leaking goroutines on close Jun 28, 2024
@liggitt
Copy link
Contributor Author

liggitt commented Jun 28, 2024

cc @dims @aojea

connection.go Show resolved Hide resolved
connection.go Show resolved Hide resolved
Copy link
Collaborator

@dims dims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dims
Copy link
Collaborator

dims commented Jul 22, 2024

@thaJeztah ok to merge and cut a tag with this change?

@thaJeztah
Copy link
Member

Fine with me, but I'm not deeply familiar, so if you need more eyes, perhaps @dmcgowan can have a quick peek (otherwise feel free to move ahead)

@dmcgowan dmcgowan merged commit 77eb080 into moby:master Jul 23, 2024
4 checks passed
@dims
Copy link
Collaborator

dims commented Jul 24, 2024

thanks @dmcgowan

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What's the purpose of the 10 minute wait?
5 participants