fix 3 bugs responsible for a goroutine leak (plus one other bug) #7491
Conversation
We probably don't need to do this, but it can't hurt.
Previously, we'd return the error + result, then the result.
| @@ -86,16 +86,19 @@ func (ns *mpns) Resolve(ctx context.Context, name string, options ...opts.Resolv | |||
| } | |||
There was a problem hiding this comment.
Could someone carefully check the changes I made here. I think they're right, but this is tricky code.
| ctx, cancel := context.WithCancel(ctx) | ||
| defer cancel() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
fix 3 bugs responsible for a goroutine leak (plus one other bug) This commit was moved from ipfs/kubo@538174f
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.api.Name().Search()but also weren't canceling the context.The last bug fix (closing the response channel) was actually my first attempt to fix the bug, but it's related to this issue.