Skip to content
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

fix 3 bugs responsible for a goroutine leak (plus one other bug) #7491

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

Stebalien
Copy link
Member

Alone, these three bugs were benign. But all together, they'd cause us to slowly leak name.Search() goroutines when resolving a name failed in some cases.

  • We were using the wrong context to resolve dnslink records.
  • We weren't reading the entire channel returned by api.Name().Search() but also weren't canceling the context.
  • We were returning results after returning errors. If we were canceling the context, or reading the entire channel, this would have been fine. But we shouldn't be doing this anyways.

The last bug fix (closing the response channel) was actually my first attempt to fix the bug, but it's related to this issue.

@Stebalien Stebalien requested review from lidel and magik6k June 18, 2020 03:01
@@ -86,16 +86,19 @@ func (ns *mpns) Resolve(ctx context.Context, name string, options ...opts.Resolv
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Could someone carefully check the changes I made here. I think they're right, but this is tricky code.

@Stebalien Stebalien requested a review from willscott June 19, 2020 05:45
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +123 to +124
ctx, cancel := context.WithCancel(ctx)
defer cancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't really do anything. Was the intent here more a sanity check? It might mask errors in api.Search which is either a good or bad thing depending on how you look at it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that we bail early from calling api.Search without reading the last result. We could say "search shouldn't return anything after the first error" but we don't currently make that guarantee.

Copy link
Contributor

Choose a reason for hiding this comment

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

k, makes sense. Is the fact that api.Search, or really the implementations of ResolveAsync, don't abort when they hit an error a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually sure. I started digging into this and hit a ton of other bugs here. I'm hoping we can merge ipfs/interface-go-ipfs-core#62, then fix the other issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

@Stebalien Stebalien merged commit 538174f into release-v0.6.0 Jun 19, 2020
@Stebalien Stebalien deleted the fix/ipns-resolve-leak branch June 19, 2020 17:05
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
fix 3 bugs responsible for a goroutine leak (plus one other bug)

This commit was moved from ipfs/kubo@538174f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants