proposal: context: add WithValues

[posener]

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

1.10

Does this issue reproduce with the latest release?

yes

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

linux

What did you do?

Consider the following standard HTTP Go server stack:

1. Middlewares that extract user credentials and request id from the headers and inject them to the http.Request context as values.
2. The http.Handler launches an asynchronous goroutin task which needs those values from the context.
3. After launching the asynchronous task the handler returns 202 to the client, the goroutin continues to run in background.

Problem Statement

• The async task can't use the request context - it is cancelled as the http.Handler returns.
• There is no way to use the context values in the async task.
• Specially if those values are used automatically with client roundtrippers (extract the request id from the context and inject it to http headers in a following request.)

Proposal

I suggest to add a function that combines values from two contexts - context.WithValues(ctx, values) or so.
An initial implementation is available here.

Discussion is available in golang-nuts forum

[rsc]

See also #19643. /cc @Sajmani @zombiezen @bcmills

[bcmills]

Some values in the Context may have finite lifetimes after which they can no longer be used. (For example, when the End method of an OpenCensus Spans is called, it is flushed to the Exporters, and future annotations to the span will be lost.)

In such cases, you need something more than just to carry over the values: you need to filter out values that can no longer be used, or perhaps extend their lifetimes.

That sort of use-case doesn't seem to be addressed in this proposal.

[Sajmani]

I don't think WithValues needs to encode that filtering; that can be done outside the context package. I think WithValues should have a somewhat more restricted signature than is in the proposal: the second parameter should have type context.Values, an interface that has only the Value method. Then users can define their own filter as a Values implementation that passes through only the allowed Value calls.

…

On Fri, Aug 10, 2018 at 2:00 PM Bryan C. Mills ***@***.***> wrote: Some values in the Context may have finite lifetimes after which they can no longer be used. (For example, when the End method of an OpenCensus Span s <https://godoc.org/go.opencensus.io/trace#Span> is called, it is flushed to the Exporters, and future annotations to the span will be lost.) In such cases, you need something more than just to carry over the values: you need to filter out values that can no longer be used, or perhaps extend their lifetimes. That sort of use-case doesn't seem to be addressed in this proposal. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26691 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJSK3Sr2PqPMk867ewvacDteeUmAK9vVks5uPcpJgaJpZM4VmQEQ> .

[posener]

Some values in the Context may have finite lifetimes after which they can no longer be used. (For example, when the End method of an OpenCensus Spans is called, it is flushed to the Exporters, and future annotations to the span will be lost.)

@bcmills can you elaborate on the given example, for those who are not familiar with the code - as it seems to me, if there is an explicit call to End, you can override values with nil value. In the case of spans, I think that we will want to record calls, even after the handler was finished? So we could see related propagation of the request even in asynchronous task.

And generally, what sort of cases need filtering out values from the context?

[bcmills]

it seems to me, if there is an explicit call to End, you can override values with nil value.

Overrides of context values are generally scoped to subtrees of the function call tree: context.WithValue produces an immutable entry in the tree.

In the case of spans, I think that we will want to record calls, even after the handler was finished?

Right, that's the problem: when End is called, the Span is flushed out to the log and nothing more can be recorded for it.

[posener]

I'm sorry, I don't understand how spans are bad example here:

1. In the same manner that you you don't close channel twice, you should not sent spans twice - you'll get an error (for example, a span manager pointed by the context could hold a state).
2. As far as I can understand the same issue could arouse In the current design of spans - I can send them several times manually.
3. I guess that the common case is to have a middleware that will handle the spans, and it will remain unharmed with this change.

For long running tasks that were created from HTTP handlers - I think it is bad practice to create a new context for them, and it is important for them to know the context that they came from. The examples I gave demonstrate the need, and they don't have a solution currently in the standard library.

[bcmills]

I guess that the common case is to have a middleware that will handle the spans, and it will remain unharmed with this change.

The middleware is exactly the point: at some point, that middleware needs to know when to flush the span, and at the moment the only reasonable signal it gets is “the next handler down the chain has returned”.

[posener]

I guess that the common case is to have a middleware that will handle the spans, and it will remain unharmed with this change.

The middleware is exactly the point: at some point, that middleware needs to know when to flush the span, and at the moment the only reasonable signal it gets is “the next handler down the chain has returned”.

Maybe asynchronous task trace should not be the same as the handler itself trace. Maybe if you want to send spans from asynchronous task, you should create a new trace and send it when you are done?

[bcmills]

Maybe if you want to send spans from asynchronous task, you should create a new trace and send it when you are done?

Yes, that's exactly my point: if the trace may be stored and accessed in the Values of a Context, then it is inappropriate to propagate those values to an asynchronous task.

[rsc]

This need can be satisfied outside the standard library, as you have. (https://github.com/posener/ctxutil/blob/master/values.go)

There are various things you might want to do to mix parts of two contexts. It's not clear which one is "right" if we're going to have a blessed mixer. Better for users who need a specific kind of mix to implement it themselves. This is why context.Context is an interface.

[bcmills]

This is why context.Context is an interface.

Note that any context.Context implementation outside the standard library imposes an extra goroutine every time context.WithCancel or context.WithDeadline is called on it.

If we're going to press for folks to address these sorts of issues outside the standard library, we should probably make the standard library play more nicely with external implementations.