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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Jun 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00de344
fix: close resolve channel before returning it
Stebalien Jun 18, 2020
5f19f3b
fix: cancel resolve search context
Stebalien Jun 18, 2020
84341d0
fix: use the correct context when resolving dnsaddr links
Stebalien Jun 18, 2020
0f3bc65
fix: return results from resolve once
Stebalien Jun 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/coreapi/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ func (api *NameAPI) Search(ctx context.Context, name string, opts ...caopts.Name
// Resolve attempts to resolve the newest version of the specified name and
// returns its path.
func (api *NameAPI) Resolve(ctx context.Context, name string, opts ...caopts.NameResolveOption) (path.Path, error) {
ctx, cancel := context.WithCancel(ctx)
defer cancel()
Comment on lines +123 to +124
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


results, err := api.Search(ctx, name, opts...)
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions core/corehttp/hostname.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func HostnameOption() ServeOption {
// Not a whitelisted path

// Try DNSLink, if it was not explicitly disabled for the hostname
if !gw.NoDNSLink && isDNSLinkRequest(n.Context(), coreApi, r) {
if !gw.NoDNSLink && isDNSLinkRequest(r.Context(), coreApi, r) {
// rewrite path and handle as DNSLink
r.URL.Path = "/ipns/" + stripPort(r.Host) + r.URL.Path
childMux.ServeHTTP(w, r)
Expand Down Expand Up @@ -176,7 +176,7 @@ func HostnameOption() ServeOption {
// 1. is wildcard DNSLink enabled (Gateway.NoDNSLink=false)?
// 2. does Host header include a fully qualified domain name (FQDN)?
// 3. does DNSLink record exist in DNS?
if !cfg.Gateway.NoDNSLink && isDNSLinkRequest(n.Context(), coreApi, r) {
if !cfg.Gateway.NoDNSLink && isDNSLinkRequest(r.Context(), coreApi, r) {
// rewrite path and handle as DNSLink
r.URL.Path = "/ipns/" + stripPort(r.Host) + r.URL.Path
childMux.ServeHTTP(w, r)
Expand Down
20 changes: 9 additions & 11 deletions namesys/namesys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


func (ns *mpns) ResolveAsync(ctx context.Context, name string, options ...opts.ResolveOpt) <-chan Result {
res := make(chan Result, 1)
if strings.HasPrefix(name, "/ipfs/") {
p, err := path.ParsePath(name)
res := make(chan Result, 1)
res <- Result{p, err}
close(res)
return res
}

if !strings.HasPrefix(name, "/") {
p, err := path.ParsePath("/ipfs/" + name)
res := make(chan Result, 1)
res <- Result{p, err}
close(res)
return res
}

Expand All @@ -120,15 +123,12 @@ func (ns *mpns) resolveOnceAsync(ctx context.Context, name string, options opts.
key := segments[2]

if p, ok := ns.cacheGet(key); ok {
var err error
if len(segments) > 3 {
var err error
p, err = path.FromSegments("", strings.TrimRight(p.String(), "/"), segments[3])
if err != nil {
emitOnceResult(ctx, out, onceResult{value: p, err: err})
}
}

out <- onceResult{value: p}
out <- onceResult{value: p, err: err}
close(out)
return out
}
Expand Down Expand Up @@ -180,17 +180,15 @@ func (ns *mpns) resolveOnceAsync(ctx context.Context, name string, options opts.
best = res
}
p := res.value
err := res.err
ttl := res.ttl

// Attach rest of the path
if len(segments) > 3 {
var err error
p, err = path.FromSegments("", strings.TrimRight(p.String(), "/"), segments[3])
if err != nil {
emitOnceResult(ctx, out, onceResult{value: p, ttl: res.ttl, err: err})
}
}

emitOnceResult(ctx, out, onceResult{value: p, ttl: res.ttl, err: res.err})
emitOnceResult(ctx, out, onceResult{value: p, ttl: ttl, err: err})
case <-ctx.Done():
return
}
Expand Down