-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Close peerstore and document Host Close #1037
Conversation
LGTM 👍 |
ping @Stebalien |
@@ -52,7 +52,7 @@ func ChainOptions(opts ...Option) Option { | |||
// - If no peerstore is provided, the host is initialized with an empty | |||
// peerstore. | |||
// | |||
// Canceling the passed context will stop the returned libp2p node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the only correct order to call these actions? What happens if we call Close before we cancel the context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raulk The order is NOT important. I've updated the PR.
ping @Stebalien @raulk for approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to remember that we did it this way because we don't "own" the peerstore, it may be shared. However, closing this is probably strictly better than what we currently do.
Nit: I'd use:
if pstore := h.Peerstore(); pstore != nil {
pstore.Close()
}
For #1030.