Skip to content
This repository has been archived by the owner on May 11, 2022. It is now read-only.

remove context from constructor, implement a proper Close method #109

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

marten-seemann
Copy link
Contributor

I wish we didn't have to make this change, but controlling shutdowns with a context simply doesn't work in Go: There's no way to know when a function listening on ctx.Done() has actually shut down. All you can hope for is that it eventually does.

A reliable way of implementing a clean shutdown is by having a Close method, which explicitly blocks until all go routines have finished.

Unfortunately, this is an API-breaking change, since we're 1. removing the ctx from the constructor, and (logically) we're adding a Close method that users of this library will now have to call explicitly.

The concrete reason why I'm sending this PR now is that this is blocking libp2p/go-libp2p#1168. In specific, we have a few tests there that modify manet.Private4. This global variable is accessed by the background loop in

if manet.IsPublicAddr(conn.RemoteMultiaddr()) &&
, creating a race condition.

autonat.go Show resolved Hide resolved
svc.go Outdated Show resolved Hide resolved
@marten-seemann marten-seemann merged commit 1247ac6 into master Aug 30, 2021
@marten-seemann marten-seemann deleted the clean-shutdown branch August 30, 2021 09:46
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants