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

net: Resolver.LookIPAddr() drops context values #28600

Closed
WinstonPrivacy opened this issue Nov 5, 2018 · 12 comments
Closed

net: Resolver.LookIPAddr() drops context values #28600

WinstonPrivacy opened this issue Nov 5, 2018 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@WinstonPrivacy
Copy link

What version of Go are you using (go version)?

v1.11.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Linux, ARM64

What did you do?

Upgraded from Go 1.9.7 to 1.11.2. Functional and unit tests immediately threw a variety of errors relating to DNS lookups. This was isolated down to a routine in net/lookup.go/LookupIPAddr().

What did you expect to see?

We use WithValue to attach values to the context object which are then consumed by our custom DNS resolvers. Any values attached to the context object sent to this routine should be passed through it.

What did you see instead?

Context values are correctly sent to LookupIPAddr() but do not leave it. As a result, our custom DNS resolvers are misbehaving because they never see the expected values in the context object which is passed on to them.

It appears that a new context object is now being created using context.Background() so the passed in values are not retained.

func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]IPAddr, error) {

...

// We don't want a cancelation of ctx to affect the

// lookupGroup operation. Otherwise if our context gets

// canceled it might cause an error to be returned to a lookup

// using a completely different context.

fmt.Println("[DEBUG] net/lookup.go/LookupIPAddr() 3 - ctx", ctx, host)

lookupGroupCtx, lookupGroupCancel := context.WithCancel(context.Background())

@ianlancetaylor ianlancetaylor changed the title Resolver.LookIPAddr() drops context values net: Resolver.LookIPAddr() drops context values Nov 5, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 5, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Nov 5, 2018
@odeke-em
Copy link
Member

odeke-em commented Nov 7, 2018

Thank you for this report @WinstonPrivacy!

As the comment in

go/src/net/lookup.go

Lines 231 to 235 in ac277d9

// We don't want a cancelation of ctx to affect the
// lookupGroup operation. Otherwise if our context gets
// canceled it might cause an error to be returned to a lookup
// using a completely different context.
lookupGroupCtx, lookupGroupCancel := context.WithCancel(context.Background())

states we are avoiding context cancelation.

I believe that the solution to this issue could benefit from @johanbrandhorst's #28279 and @rsc's recommendation #28279 (comment): since context.Context is an interface, just having a front facing context that falls back to the old context for value look up, but will return a fresh channel from Done which will solve the quagmire of avoiding cancelation

@WinstonPrivacy
Copy link
Author

WinstonPrivacy commented Nov 7, 2018 via email

@odeke-em odeke-em self-assigned this Nov 7, 2018
@johanbrandhorst
Copy link
Member

johanbrandhorst commented Nov 7, 2018

Interesting find and conclusions @odeke-em. I'm wondering now since the stdlib is plagued by this as well if the decision to exclude such an implementation from the context package should be revisited? Of course, a custom context implementation in net is the other solution.

@gopherbot
Copy link

Change https://golang.org/cl/148698 mentions this issue: net: LookupIPAddr should preserve context values for Resolver

@odeke-em
Copy link
Member

odeke-em commented Nov 9, 2018

I'm wondering now since the stdlib is plagued by this as well if the decision to exclude such an implementation from the context package should be revisited? Of course, a custom context implementation in net is the other solution.

@johanbrandhorst for now, I think we can present the custom implementation in net, it is a simple one that uses composition and falls back to the creating context for value lookups only.

type onlyValuesCtx struct {
   context.Context
   lookup context.Context
}

func (ovc *onlyValuesCtx) Value(key interface{}) interface{} { return ovc.lookup.Value(key) }
func WithOnlyValues(ctx context.Context) context.Context {
   return &onlyValuesCtx{Context: context.Background(), lookup: ctx}
}

In regards to adding it to the standard library I think if we can find another spot that needs this
context, you can then use that as a compelling reason to start a proposal with those concrete needs.

@ianlancetaylor
Copy link
Contributor

@odeke-em has a fix but I'm concerned about the approach.

The newly created context can outlive the original context. This will happen if the original context has a timeout shorter than the time required for the DNS lookup. Canceling the original context may in some way invalidate the values. The result will be that the resolver code is looking at a context containing invalid values.

If there are multiple simultaneous lookups of a single IP address, and the lookups use different contexts with different values, then only one set of values will be passed to the DNS lookup. Whether this is correct or not depends on what those values mean.

So: @WinstonPrivacy what do the values in your contexts mean?

It's too bad there is no way to ask a context whether it contains any values at all. If there were such a way then we could avoid coalescing different lookups of the same IP address when using a context containing values.

One way to partially address the first problem by changing the CL to only return a value from the underlying context if it has not been canceled. But I don't know whether this is right or wrong.

@WinstonPrivacy
Copy link
Author

WinstonPrivacy commented Nov 9, 2018 via email

@ianlancetaylor
Copy link
Contributor

@WinstonPrivacy Thanks. What is the correct behavior if goroutine 1 has a context that resolves to the local DNS service, and goroutine 2 has a context that queries the upstream DNS services directly, and goroutine 1 looks up 1.2.3.4, and, before that lookup completes, goroutine 2 also looks up 1.2.3.4?

@WinstonPrivacy
Copy link
Author

WinstonPrivacy commented Nov 9, 2018 via email

@ianlancetaylor
Copy link
Contributor

Thanks. Making it the responsibility of the caller is fine but of course nobody will ever see the documentation for this, so it needs to behave reasonably regardless.

@odeke-em I think you should modify your CL so that after retrieving the key, if the context is canceled, we pretend that the key wasn't there. I don't see any fully correct approach that we can use, and that seems the most likely to do the right thing.

Happy to hear counter-arguments.

@odeke-em
Copy link
Member

odeke-em commented Nov 9, 2018

@odeke-em I think you should modify your CL so that after retrieving the key, if the context is canceled, we pretend that the key wasn't there. I don't see any fully correct approach that we can use, and that seems the most likely to do the right thing.

Thanks for the suggestion, raising and discussing the point about expiry! Sounds like a plan, I can definitely do that.

@WinstonPrivacy
Copy link
Author

WinstonPrivacy commented Nov 13, 2018 via email

@golang golang locked and limited conversation to collaborators Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants