proposal: context: new package for standard library #14660

Closed
bradfitz opened this Issue Mar 5, 2016 · 67 comments

Projects

None yet
@bradfitz
Member
bradfitz commented Mar 5, 2016

@Sajmani and I are interested in adding golang.org/x/net/context to the standard library, probably as the top-level package context.

(Why not net/context? It has little to do with networking, except that people often use it with networking; it doesn't even cross network boundaries on its own without help.)

Some places in the standard library which could use it:

  • https://golang.org/pkg/net/http/#Request -- add a new Context field, alongside like the existing (Context.Done-compatible) Request.Cancel field. It will take precedence over Cancel, and we can make it work for both outbound and inbound requests. For server requests it would use CloseNotifier's mechanism for c* ancelation. (which for http2 includes the http2-level explicit stream cancelation frame too, also used by gRPC)
  • https://golang.org/pkg/net/#Dialer -- same as http.Request. net.Dialer also has a Done-compatible Cancel field, which could be a Context.
  • https://golang.org/pkg/os/exec/#Cmd could have a new Context field to automatically kill+wait the process when a context expires, if the context expires before the child?
  • https://golang.org/pkg/database/sql/#DB.Query already takes ...interface{} which can be special-cased to say that if the first parameter is a context.Context, it's used for deadlines. Trashy or clever? Maybe both. We could also add parallel DB.QueryContext which is a bit of a mouthful.
  • .... (where else?) ....

Open questions:

  • do we do something with the io package in the same release, or later? i.e. do we add *os.File.IO(context.Context) io.ReadWriteCloser sort of methods, to curry a context into io interfaces? Or wait on that. If it'd be nice to push down cancelation into even OS-level file I/O, but we don't even really do that inside Google with IO-like methods and pervasive Context usage, since we don't really use *os.File. So I'm inclined to wait on that. Too many operating systems to deal with.

Concerns:

  • we can't add it to as many places in the standard library we'd like to since APIs are frozen. Basically we can only add it to structs. While we've told people not to add contexts to structs, I think that guidance is over-aggressive. The real advice is not to store contexts. They should be passed along like parameters. But if the struct is essentially just a parameter, it's okay. I think this concern can be addressed with package-level documentation and examples.
  • what field name in structs to use? We use "ctx" for variables, but struct names are typically spelled out. But does that cause too much stutter? e.g.
package http
type Request struct {
    Context context.Context
}
@bradfitz bradfitz added the Proposal label Mar 5, 2016
@bradfitz bradfitz self-assigned this Mar 5, 2016
@elithrar
elithrar commented Mar 5, 2016

For others following, there was a substantial discussion on golang-dev about this as it related to net/http: https://groups.google.com/forum/#!topic/golang-dev/cQs1z9LrJDU

As for naming: the only stutter is in the field name in the source. Package users are going to call r.Context.WithValue or r.Context.Done, which has the benefit of being explicit and (say you point out, Brad) retains the ctx naming convention for variables.

Looking forward to this happening!

@atdiar
atdiar commented Mar 5, 2016

Thinking about http.Handler, people can alternatively rewrite a different kind of handler that has a different signature for ServeHttp. The advantage is that one can easily add a Context parameter.
But also, add return values. For instance a boolean sentinel value to control whether the handler should be the last one to be executed. Or a different http.ResponseWriter (useful if you wrap the ResponseWriter in one of your handlers, for instance when you gzip things, but not only)

The top level mux object that encapsulates the whole request dispatching logic can still remain a regular http.Handler (and the context.Background would be instantiated in its ServeHttp(w,r) and passed to the handler ServeWithCtx(ctx,w,r)). Probably better that I upload an example of what I mean.

There are also a few things that I am not very fond of. context.TODO is one of them. But I don't know if it is too late to change things or not.
Is this the right place to discuss about API design ?

@jimmyfrasche

I think naming it Context in structs is good. There's a stutter, but only for the author of the API. Couldn't be clearer for the users of the API.

Would it be possible to write a go vet check for obvious misuse of a Context in a struct? If so, it would be great to roll that out at the same time. Posting a sign next to the pool is good, but a lifeguard is better.

As much as I'd love Context to be deeply baked into the standard library, I'd be happy with it simply being added to the standard library to say "this is blessed, find good ways to use it" and later adding the best ideas to the stdlib where they can be and kept in third party packages where they cannot. However, I'm not opposed to any of the suggested integrations.

Having the first interface{} parameter to Query be a Context is clever, though, so I'd much rather have the explicit QueryContext.

@riannucci

This is maybe a bit out of place for this particular issue, but I think it would be worth considering a different Context implementation (strawman benchmark) before it gets included in the stdlib.

In that benchmark, I compare a bogus map+lock "caching" context implementation to the default implementation. Even with as few as 10 items, there's a noticeable improvement (%-wise, not in absolute terms. I realize we're talking nanoseconds here) on lookups into the cached version. For contexts deeper than 10 items, there's a noticeable improvement. The construction overhead is similar, but I think with a non-bogus implementation the map version could be even faster for overhead than the linked list one.

I think there's an opportunity to have Context be a *struct-with-hidden-fields instead of an interface, which would afford the opportunity to build a faster implementation of it (e.g. by having .WithValue be a struct method instead of a package method that can be smart about chaining, or a .WithValues to avoid the linked-list-building overhead when adding multiple items to the context at the same time) and by possibly adding a .Compact method to compactify an existing Context to make subsequent lookups fast. It's not possible to build these things in a clean fashion with the current interface-based implementation (since the only way for the context to see up the chain is to actually do lookups, and the only way to know the lookup keys is to wait for some user code to provide them).

I'm mostly thinking about the pattern of:

func MyHTTPHandler(rw ResponseWriter, r *Request) {
  c := context.Background()
  // WithValue 10-30 services/contextful things here... this number will likely grow
  // as context becomes even more prevalently used.

  // 100's of lookups into c here... if it were compacted here then all the lookups
  // would be O(1).
  myActualHandler(c, rw, r) 
}

Which, I think, is a pretty common pattern (I don't have data on hand for this assumption, however).

In general, for most stdlib-type libraries, there would simply be competition for the best implementation. But I think that the community is rallying around a single Context implementation (because it's THE way to build composable/stackable libraries which are loosely coupled). There can't really be an alternate implementation that doesn't stem from the stdlib (or x/) that has any hope for survival (since everything needs to adopt the stdlib one to actually be used by anything). To be clear: I think it's GOOD for the community to standardize here, I just think it might be worth considering a slightly speedier implementation before cooking it into the 1.x API compatibility guarantee.

Note that the WithTimeout and WithDeadline package functions are well-served by the interface implementation right now: it allows e.g. mocking of the clock. Some similarly mockable interface would need to be built for that functionality, but AFAIK that mocking requirement doesn't really apply to WithValue.

edit: typo

@kr
Contributor
kr commented Mar 5, 2016

How do you expect putting context in http.Request (or any other parameter-ish struct) will interact with adding values (or timeouts or whatever) to the context?

I can think of two basic approaches, caller-save and callee-save:

func f(w http.ResponseWriter, req *http.Request) {
    ctx := req.Context
    g(w, req)
    // ... something with ctx ...
}
func g(w http.ResponseWriter, req *http.Request) {
    req.Context = context.WithValue(req.Context, foo, bar)
    h(w, req)
}

or

func f(w http.ResponseWriter, req *http.Request) {
    g(w, req)
    // ... something with req.Context ...
}
func g(w http.ResponseWriter, req *http.Request) {
    req2 := new(http.Request)
    *req2 = *req
    req2.Context = context.WithValue(req.Context, foo, bar)
    h(w, req2)
}

Of course, it's fine if both caller and callee save the old value, but it's bad if neither of them does it, each expecting the other to do so. So, should we recommend one and document and promote it up front?

@rakyll
Member
rakyll commented Mar 5, 2016

Traditionally, context objects should carry configuration. The x/net/context package extends context objects' responsibilities by providing constructs for timeout and cancellation signals. In the scope of stamp coupling of request handlers, it is a valuable addition. But I am not sure how it will scale to the rest of the standard library and furthermore whether the context may end up being abused for signaling purposes. The two cases you mention above (exec.Cmd and sql.DB.Query) does this by solely using a context object for handling signals even though there is no necessity for storage. Are you trying to solve the issue of how data is passed among functions or also trying to have primitives around how the entire execution could be signaled from outside?

One of the concerns I would add to the list is that the context should be scoped to a function call.

"Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx" (link)

If the standard library won't be agreeing with this due to compatibility reasons, don't you think it will create confusion?

@bradfitz
Member
bradfitz commented Mar 6, 2016

@rakyll, I addressed that final concern in my first post. ("While we've told people...")

@olivere olivere referenced this issue in olivere/elastic Mar 6, 2016
Closed

net/context support #239

@dsymonds dsymonds added this to the Go1.7Early milestone Mar 6, 2016
@zippoxer
zippoxer commented Mar 7, 2016

If there's any way to add context to the standard library without making http or net aware of context (can still adjust them to play well with it though), this would be the way to not overwhelm newcomers reading the docs.

@okdave
Contributor
okdave commented Mar 7, 2016

The plan @bradfitz is proposing makes http and net aware of context, but not in a way that requires their use for basic cases. The http package is already very large, but you only need to use a very small fraction of it to achieve "hello world"-esque examples. Adding context to these packages won't harm newcomers, and should make life easier in more complicated/large codebases.

@bradfitz
Member
bradfitz commented Mar 8, 2016

I mailed out https://golang.org/cl/20346 &https://golang.org/cl/20347

The first copies x/net/context to the standard library. The second reworks x/net/context to just use the standard library's version.

@gopherbot

CL https://golang.org/cl/20346 mentions this issue.

@gopherbot

CL https://golang.org/cl/20347 mentions this issue.

@bradfitz
Member
bradfitz commented Mar 8, 2016

@kr, making a new request (req2 := new(Request) ...) is probably the safer option. You don't know whether one of your callees is still using the original Request.

@bradfitz
Member
bradfitz commented Mar 8, 2016

@riannucci, I think redesigning Context is too big and out of scope. We have tons of code already using the existing Context and we want to make migration easy.

@riannucci

@bradfitz yeah, I kinda figured as much, but I was hoping that since it's only in x now and not in the stdlib, we still had some API flexibility (e.g. introduce new one to stdlib, deprecate x version, remove x version in 1.8-era or something like that)

@chowey
chowey commented Mar 12, 2016

I would love to see support for Context in text/template, such that:

ctx := context.WithValue(context.TODO, "Title", "Through the Looking Glass")
template.Must(template.Parse(`Title: {{.Title}}`)).Execute(os.Stdout, ctx)

would output something like:

Title: Through the Looking Glass

If we are worried about accidentally leaking stuff from the Context into a template (because you used a string as a key elsewhere), then I propose adding a special type to distinguish values you want exposed:

// in the text/template package
type Expose string
// in your program
ctx := context.WithValue(context.TODO, template.Expose("Title"), "Through the Looking Glass")
template.Must(template.Parse(`Title: {{.Title}}`)).Execute(os.Stdout, ctx)

This way, you are deliberate about exposing values to the template.

@nodirt
Member
nodirt commented Mar 12, 2016

@chowey: what problem you are trying to solve? If keys have to be dynamic, why not to use a map?

@atdiar
atdiar commented Mar 14, 2016

@bradfitz @riannucci yes, that's my sentiment too. I understand though that x/context is widely used but since it's about to be frozen into the stdlib, I'd rather have the simplest interface possible. Not only for the current users but the ones that have yet to come. I will still understand if it's difficult from the POV of logistics but...

@elithrar

What do you find 'not simple' about the current net/context API? The most
basic use-case revolves around WithValue / Value, which are (I think!)
fairly straightforward.

As Brad says though, it's probably a little too late to change anything:
although it's an "x" package, it's been in the wild for a long while.

On Mon, Mar 14, 2016 at 6:36 AM atdiar notifications@github.com wrote:

@bradfitz https://github.com/bradfitz @riannucci
https://github.com/riannucci yes, that's my sentiment too. I understand
though that x/context is widely used but since it's about to be frozen into
the stdlib, I'd rather have the simplest interface possible. Not only for
the current users but the ones that have yet to come. I will still
understand if it's difficult from the POV of logistics but...

โ€”
Reply to this email directly or view it on GitHub
#14660 (comment).

@atdiar
atdiar commented Mar 14, 2016

@elithrar well, the most interesting use is the propagation/monitoring of cancellation signals, not really WithValue (even though it helps for handler chaining).
I don't find WithValue very ergonomic compared to passing a mutex protected map.
If we have to have a canonical Context, we don't really need it to be an interface either. A struct with hidden fields suffices.
Also, WithTimeout and WithDeadline are somewhat doing the same. You could remove one of those. That would make things already a bit simpler.

I understand what Brad said. Still I'd rather the decision be clearly examined for the long term. It was in x/context and not the stdlib for a reason.

context.TODO was probably supposed to be for static analysis but I think it is not needed.

@bobziuchkovski

I share @atdiar 's concerns. While I understand how golang.org/x/net/context works, it took me multiple reads through the godocs and blog post for it to click. I find the current API kind of confusing.

I threw together an alternative implementation (albeit in a rush) with what I believe is a simpler API and easier learning curve: https://github.com/bobziuchkovski/context, godocs at https://godoc.org/github.com/bobziuchkovski/context

I realize it's a bit late in the game, but I feel it's at least worth discussing the current x/net/context API before it hits stdlib.

@elithrar

@bobziuchkovski For posterity, can you update your comment (or add a new one) to clarify the pros/cons of your lib vs. the existing x/net/context? I can see that Terminated() and TimeRemaining() are new interface methods (with clear uses), but what are the downsides here?

@bobziuchkovski

Sure. Terminated() and TimeRemaining() are mostly synonymous with the existing Done() and Deadline(). In terms of the changes there, I think Terminated() better conveys that the context was actually canceled. I believe a x/net/context could finish it's work and be "done" but the Done() channel wouldn't close unless an expiration or CancelFunc were fired.

For TimeRemaining() vs Deadline(), there's more value (for me anyway) in knowing how much time is left vs an expiration date. I can't personally think of a case where I'd want the Deadline() date vs simply knowing how much time (if any) is left. This isn't a major concern, but just something to discuss.

Adding the WithValue and WithTimeout methods to the interface, like I've done, definitely makes it "fatter", so that could be a downside. I believe it makes the parent/child relationships clearer and makes the context cleaner to construct/work with than all of the different x/net/context.With* functions, but that might just be me.

I think my biggest concern with the current x/net/context is the CancelFunc stuff and the Background() and TODO() funcs. I "get it" now, but those were the biggest barrier to my understanding of the package. It's far easier for me to comprehend Context.Cancel() and implicit cancelation via timeout with the implementation I provided than all of the CancelFunc return values from x/net/context. Similarly, it's easier for me to comprehend creating new contexts via New(), Context.WithTimeout(), and Context.WithValue() than using TODO(), Background() and the different WithValue(), WithDeadline(), WithTimeout(), and WithCancel() constructor funcs from x/net/context. On the one hand, I get that using those keeps the interface slimmer, but on the other hand, it makes x/net/context far more confusing (to me).

I'm not necessarily putting forth this alternative implementation as the alternative, but rather an alternative to hopefully spark some discussion around the current x/net/context API.

@elithrar

Thanks Bob.

I think thatโ€”given the recurring theme of 'net/context not being clear
about usage'โ€”some of this could be addressed through documentation, if API
changes aren't palatable. It's not the first time I've seen confusion about
how it works/how to use it across the mailing list/SO/forums/etc.

(to address another comment: net/context exists to replace mutex-protected
maps)

On Mon, Mar 14, 2016 at 9:48 AM Bob Ziuchkovski notifications@github.com
wrote:

Sure. Terminated() and TimeRemaining() are mostly synonymous with the
existing Done() and Deadline(). In terms of the changes there, I think
Terminated() better conveys that the context was actually canceled. I
believe a x/net/context could finish it's work and be "done" but the
Done() channel wouldn't close unless an expiration or CancelFunc were
fired.

For TimeRemaining() vs Deadline(), there's more value (for me anyway) in
knowing how much time is left vs an expiration date. I can't personally
think of a case where I'd want the Deadline() date vs simply knowing how
much time (if any) is left. This isn't a major concern, but just something
to discuss.

Adding the WithValue and WithTimeout methods to the interface, like I've
done, definitely makes it "fatter", so that could be a downside. I believe
it makes the parent/child relationships clearer and makes the context
cleaner to construct/work with than all of the different
x/net/context.With* functions, but that might just be me.

I think my biggest concern with the current x/net/context is the
CancelFunc stuff and the Background() and TODO() funcs. I "get it" now, but
those were the biggest barrier to my understanding of the package. It's far
easier for me to comprehend Context.Cancel() and implicit cancelation via
timeout with the implementation I provided than all of the CancelFunc
return values from x/net/context. Similarly, it's easier for me to
comprehend creating new contexts via New and Context.WithTimeout() and
Context.WithValue than using TODO(), Background() and the different
WithValue(), WithDeadline(), WithTimeout(), and WithCancel() constructor
funcs from x/net/context. On the one hand, I get that using those keeps the
interface slimmer, but on the other hand, it makes x/net/context far more
confusing (to me).

I'm not necessarily putting forth this alternative implementation as the
alternative, but rather an alternative to hopefully spark some
discussion around the current x/net/context API.

โ€”
Reply to this email directly or view it on GitHub
#14660 (comment).

@bobziuchkovski

@elithrar I definitely agree there. Even without API changes, a lot of my concerns could be addressed via documentation.

@tv42
tv42 commented Mar 15, 2016

@bobziuchkovski Cancel being a method on context lets functions the context is passed to cancel the context of their caller, which sounds like a bad thing. In fact, doing the right thing requires explicit effort with that API. WithCancel returning the cancel function separately means you get to (have to) choose who has that power.

@bobziuchkovski

@tv42 Yeah, that occurred to me, but if that's the reason for the current API, that seems like a lot of engineering effort to solve a problem where "don't do that" might work. ๐Ÿ˜„ More specifically, "don't cancel a context you didn't create". I don't know.

I can understand the argument that it's hard to change the API after it's been in the wild for some time. But I can also honestly say that it took me genuine head scratching and re-reading of the godocs and blog post multiple times before it clicked.

It might be as Matt suggested and just require some additional documentation. Or it might be something where some API tweaks would honestly help. I feel a bit bad for being a nay-sayer, but that's also why I tried to contribute some example code as a means of providing constructive feedback.

@schmichael

https://godoc.org/golang.org/x/net/context hints at the existence of a static analysis tool, yet I see no mention of one here or in linked discussions or CLs.

Is a static analysis tool considered out of scope for the initial stdlib inclusion of context? That seems reasonable but given concerns over type safety and the doc references, I kind of expected a tool would at least get mentioned in the proposal.

@rsc rsc changed the title from Proposal: add context to the standard library to proposal: context: new package for standard library Mar 23, 2016
@rsc
Contributor
rsc commented Mar 23, 2016

My main concern here is presenting clear advice about structs. The standard library should demonstrate the idiomatic best practices for using context, or else no one else will use context idiomatically either. That said, I do agree with Brad that - if done clearly - we can relax the struct prohibition a bit.

Brad wrote:

While we've told people not to add contexts to structs, I think that guidance is over-aggressive. The real advice is not to store contexts. They should be passed along like parameters. But if the struct is essentially just a parameter, it's okay. I think this concern can be addressed with package-level documentation and examples.

I think this is correct. net.Dialer and exec.Cmd are clearly "parameter structs" in that from the start they contained things that only made sense for a single operation (Deadline for net.Dialer, results like Process for exec.Cmd).

For sql.DB, overloading the args ...interface{} list seems pretty hacky, but the alternative is a whole new parallel API, which is almost as bad. If we're going to do that, though, it seems like maybe it should be the last arg instead of the first. At least then any references to positional arguments continue to use the same indexes, and it avoids the context being a middle argument in calls to functions that take 'query string, args ...interface{}'.

Let's leave os.File out of it for now. Most operating systems don't support timeouts on file reads anyway, and os.File.IO seems very clearly storage of a context.

A field 'Context context.Context' is fine. I do wonder if maybe Context should be in package io. I've suggested this before but I don't remember if there was a reason it can't be done. It's not about reader/writer io but it's certainly almost always some kind of input or output operation, and io.Context is not so stuttery.

The biggest problem is going to be http.Request. Once you know that Contexts must only be passed in parameter-like structs, that implies http.Request must be a parameter-like struct and so the answer to @kr's excellent question has to be that you make a shallow copy of the entire request to pass along a different context, as Brad said. I think this will surprise people. It's actually already the case that http.Request is parameter-like and that programs should only be modifying copies of the http.Request, not the original, but Context will make that even more important, and it will be a surprise. It will need very clear documentation.

But it seems OK to go forward with. I had been hoping to ask @Sajmani what his thoughts were in person, but I have not gotten a chance yet. So I will ask here: Sameer, how does this all look to you?

Thanks.

P.S. I wonder if we should employ the race detector here as well. When run under the race detector, the http server could kick off two goroutines for each request instead of one. Specifically, serverHandler.ServeHTTP could replace:

    handler.ServeHTTP(rw, req)

with

    var c chan http.Request
    if raceEnabled {
            c = make(chan http.Request)
            go func() {
                    c <- *req
            }()
    }
    handler.ServeHTTP(rw, req)
    if raceEnabled {
            <-c
    }

It's a little heavy but so is the race detector generally. The coordination on the channel makes sure that when serverHandler.ServeHTTP returns there are no pending uses of the request, but the copying of the request into the channel will conflict with any write to a field of req during handler.ServeHTTP, which should report races in handles that overwrite the original req's Context field (or others). In practice some fields are OK to write (for example fields lazily initialized, like FormValues), so this exact code won't work but the general idea of reading from unmodifiable fields in a separate goroutine under the race detector seems like it should be able to serve as a mechanical check that people are doing the right thing. Of course any common middleware layers that already make their own copies of an http.Request and then pass that further down the chain would need to add this pattern too. I don't know of any instances of that in the standard library.

@i3d
Contributor
i3d commented Mar 23, 2016

One area seems haven't been discussed is the "detach" functionality of a context. We've found legitimate use cases (at least inside Google) that sometimes detach a context to pass into an async routine within the ServeHTTP path seems useful (if not the only correct way of handling it). For example, some server logging routine will need to hold references of {req,resp} but generally have nothing to do with the end user, so the post-response logging will happen in a goroutine that potentially lasts longer than ServeHTTP.

I wonder if it is worth considering to merge the detach functionality into the context library?

@atdiar
atdiar commented Mar 24, 2016

@i3d if the context is not tightly coupled with a specific request as is being proposed, creating another new top context object (with the intended detached timeout/deadline value) should be sufficient and no additions would have to be made to the context library. Which would be the generic pattern to handle any task than needs to outlast the execution context it stems from.

If any field had to be added to a http.Request, that would be a deadline, I tend to think.

I am still concerned that if we are not careful and rethink execution contexts a little, unnecessary complexity will be introduced in the stdlib. (Sorry for being persistent about this)

@i3d
Contributor
i3d commented Mar 24, 2016

Maybe I haven't fully grasp the principle proposed here but I thought the whole "parameter" concept is to clarify that a context (at least under http.Request topic) is tightly coupled with the Request (and in general, a context should be coupled with its intended operation), that's it, it is a parameter along with the Request, and it will be invalid to use aftermath. The detach functionality is to make sure anything that still valid for the Request (e.g. user, creds, etc) are transferred into the new context but the time sensitive values (e.g. deadline, timeout duration, cancellation, etc) are not. "Creating" a brand new context for the async operation, not sure if it can work on all cases.

@atdiar
atdiar commented Mar 24, 2016

In my understanding, the context should be tightly coupled with its goroutine ideally, i.e. passed as a separate argument.
It will be clearer since that's more likely to be the idiomatic use elsewhere. ( A function that does part of its job asynchronously should accept a context.Context as argument).

The idea of creating a new top level context is to detach the cancellation signals. Indeed, any request specific values can be copied to the new context as values. That should not be a problem.

In fact, your example typically makes me realize that if the context is kept as a proper argument, things will be clearer.

Since the http APIs are already frozen, the idea was to have a field on http.Request holding a context. If it is because a cancellation signal can be received from upstream then I guess yes, it is necessary. If it is just in order to pass a context.Context with or without a deadline/timeout, better use a proper context-aware wrapper around the API, imho.

@jimmyfrasche

What if the context on Request is unexported and has a quasi-getter that only allows you to obtain the context once, like

func (r *Request) Context() context.Context {
  if r.ctx == nil {
    panic("a message explaining you've done wrong")
  }
  c := r.ctx
  r.ctx = nil
  return c
}

and a quasi-setter that returns a new copy of the Request with the ctx set, like

func (r *Request) WithContext(ctx context.Context) *Request {
   rc := r.copy() //or whatever needs to be done
   rc.ctx = ctx
   return rc
}

It would only catch some misuse but it would at least make it clear that there's something going on that you should pay attention to, whether you're looking at the documentation or code in the wild.

@chowey
chowey commented Mar 25, 2016

Why not make Request itself implement Context? After all, Context is just and interface, and adding methods will not violate the Go 1 compatibility guarantee.

@Sajmani
Contributor
Sajmani commented Mar 31, 2016

@rsc Overall the plan sounds OK. In the specific case of http.Request, I suggest we provide an API that makes it difficult to do the wrong thing. In particular, (*http.Request).Context could be a method that returns the Context, so the caller cannot change it. We provide the additional method (*http.Request).WithContext(Context) *Request to create a copy of the request with a new Context (which may be derived from the original request Context).

@extemporalgenome

@rsc https://golang.org/pkg/net/http/#StripPrefix effectively "modifies the request in-place"

@bradfitz
Member
bradfitz commented Apr 4, 2016

@Sajmani, I like that. I'll submit the context CLs (as package context for now, can be moved later) and proceed with the http and rest of the standard library plumbing. I have a bunch of other bugs which would be much easier with contexts available.

@extemporalgenome

@Sajmani @bradfitz does this mean that Request will land its first unexported field? That sounds like a major shift? Granted, in this case it has getter/setter-like functionality, kind of.

@bradfitz
Member
bradfitz commented Apr 4, 2016

@extemporalgenome, yes, I guess so. I'm not that worried about it. I don't see how it prevents anything testing-wise, for instance.

@riannucci

It might affect assignability, in case someone else has some sort of clone of the Request struct (I don't have anything in mind re: this, or any idea why anyone would do this, but it would be a change).

@bradfitz
Member
bradfitz commented Apr 4, 2016

@riannucci, I don't think it would. It would still be assignable, both as pointer and value.

@bradfitz
Member
bradfitz commented Apr 4, 2016

(we won't add a mutex or sync.once to the http.Request)

@extemporalgenome

I suspect @riannucci means in case someone tries to say: var req http.Request = struct { Method string; ... }{ ... } . In any case, adding struct fields is listed as not breaking compatibility.

Re unexported, I wasn't voicing a dissenting opinion, just a point of interest. I don't see how it'd affect testing either. With this proposal, https://godoc.org/golang.org/x/net/context/ctxhttp looks like it could go away altogether.

@riannucci

@bradfitz I think I was thinking of this: http://play.golang.org/p/gLXfzLvJlA (which doesn't compile, I picked pe.File as the first stdlib struct I could find with mixed eported/unxeported fields).

@riannucci

@extemporalgenome yeah, you're right, it's a moot point because this sort of change is explicitly blessed (and again, I don't know why anyone would have relied on this behavior before, but I've seen it happen across third-party libraries).

@gopherbot gopherbot pushed a commit to golang/net that referenced this issue Apr 5, 2016
@bradfitz bradfitz context: implement in terms of the standard library for Go 1.7+
See https://golang.org/cl/20346

Updates golang/go#14660

Change-Id: Ia974e70cdcb240ae1df0018a07595c4d1dcd422a
Reviewed-on: https://go-review.googlesource.com/20347
Reviewed-by: Sameer Ajmani <sameer@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
afc22ee
@gopherbot gopherbot pushed a commit that referenced this issue Apr 5, 2016
@bradfitz bradfitz context: add the context package from golang.org/x/net/context
This copies the golang.org/x/net/context package to the standard library.

It is imported from the x/net repo's git rev 1d9fd3b8333e (the most
recent modified to x/net/context as of 2016-03-07).

The corresponding change to x/net/context is in https://golang.org/cl/20347

Updates #14660

Change-Id: Ida14b1b7e115194d6218d9ac614548b9f41641cc
Reviewed-on: https://go-review.googlesource.com/20346
Reviewed-by: Sameer Ajmani <sameer@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
9db7ef5
@pbnjay
pbnjay commented Apr 5, 2016

I'm a bit late to the discussion, but would it make more sense to add a Context to sql.Tx instead of the Query args? Automatic rollback on cancellation would be desirable, no? Alternative, maybe less desirable version would be a method like BeginWithContext(ctx)

@gopherbot gopherbot pushed a commit that referenced this issue Apr 5, 2016
@bradfitz bradfitz net/http: add Request.Context and Request.WithContext
Currently only used by the client. The server is not yet wired up.  A
TODO remains to document how it works server-side, once implemented.

Updates #14660

Change-Id: I27c2e74198872b2720995fa8271d91de200e23d5
Reviewed-on: https://go-review.googlesource.com/21496
Reviewed-by: Andrew Gerrand <adg@golang.org>
c1c7547
@bradfitz
Member
bradfitz commented Apr 5, 2016

@pbnjay, let's move database/sql-specific discussion to the newly-created #15123.

@pkieltyka

@bradfitz btw, I noticed your note about potentially changing the baseCtx from the server for each request in cabb140

Perhaps the same or related, what do you think about having a root context for the server that is used as the parent for each request context? this way, as the server is shutting down, it can signal to every context it should finish or cancel?

@bradfitz
Member

@pkieltyka, "it should finish" sounds like graceful shutdown (#4674) but semantically, canceling a context is much more abrupt than that.

@pkieltyka

@bradfitz yea its true that canceling every in-flight context would be pretty abrupt and what I'm referring to is for purposes of a graceful shutdown.

The context tree is the one thing that could actually make it graceful though. Depends how a listener on the ctx.Done() channel considers the signal - should it cancel abruptly or just stop on the next tick? I'm pretty sure in the wild we'll see both cases. hmm.. should there be a ctx.Canceled() channel?

@riannucci

FWIW, our team has used the cancelation of contexts for graceful shutdown (e.g. wiring SIGTERM -> cancel()). It would be useful to have degrees of cancellation (e.g. STOP EVERYTHING NOW v. Stop when you can). That said, we tend to write crashsafe software, so when we want stop-everything-now, we usually don't want to rely on the application logic to implement that (and we just kill -9 it).

@bradfitz
Member

@pkieltyka, @riannucci, changing the context API and/or semantics aren't really in scope for this bug. They've been frozen for some time. I suggest we move any discussion of graceful shutdown to #4674. The answer may involve contexts in some way, but not by canceling some root context.

@gopherbot

CL https://golang.org/cl/22529 mentions this issue.

@gopherbot gopherbot pushed a commit that referenced this issue Apr 28, 2016
@bradfitz bradfitz os/exec: add Cmd.RunContext and Cmd.WaitContext
Updates #14660

Change-Id: Ifa5c97ba327ad7ceea0a9a252e3dbd9d079dae54
Reviewed-on: https://go-review.googlesource.com/22529
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
2cc27a7
@OneOfOne
Contributor

There's a typo @ 2cc27a7#diff-2f929a7660c606fd0c5b268dad1a9bd4R415

waitDone is redefined inside the if block.

@bradfitz

@bradfitz
Member

@OneOfOne, https://go-review.googlesource.com/22581. In the future, you can reply to code in Gerrit which we use for our code reviews.

@bradfitz
Member
bradfitz commented May 5, 2016

This is done, so closing. Context lives in package context for now (maybe subject to change yet, but unlikely), and is used by net, net/http, net/http/httptrace, and os/exec for now. The database/sql changes to use Context didn't happen for 1.7.

@bradfitz bradfitz closed this May 5, 2016
@shurcooL
Member
shurcooL commented May 5, 2016

Context lives in package context for now (maybe subject to change yet, but unlikely)

Are there some contending alternative names that have been suggested? I am just curious, because I could not think of any. If there was a discussion about this elsewhere, can you point to that; I'd rather not start bikeshedding here.

@minux
Member
minux commented May 5, 2016
@bradfitz
Member
bradfitz commented May 5, 2016

@minux, no rush. Eventually. Maybe at Go 1.8 time.

@phuslu
phuslu commented May 8, 2016 edited

@riannucci

I share you concern about the WithValue linked-list performance.

Now I use below pattern in my codes,

func MyHTTPHandler(rw ResponseWriter, r *Request) {
  c := context.Background()
  c = c.WithValue(c, "placeholder", &struct{Member1 string, Member2 string, Member3 int}{"foo", "bar", 42})
  myActualHandler(c, rw, r) 
}

Do you think it could help the looking up performance or not?

@Thomasdezeeuw
Contributor

@phuslu instead of guessing, why not write a benchmark?

@phuslu
phuslu commented May 9, 2016 edited

@Thomasdezeeuw Thank you.

But the more important thing(I think) is, Does this pattern is the RIGHT practice of context.Context ?

As you see, the original context.Context is used as a nested structure and seems that each node is immutable(Please correct me if I misunderstand). the original common pattern is,

ctx = context.WithValue(ctx, "placeholder", ...)

And the pattern which I used is treat context.Context as a mutable object -- although it may has potential performance improvement.
It will encourage people use below code against context.Context

ctx.Value("placeholder").(*struct {}).Member1 = ...

It smells bad. Hopefully could get comments from you and others.

@bradfitz
Member
bradfitz commented May 9, 2016

@phuslu @riannucci @Thomasdezeeuw, please move discussion of Context.WithValue performance to a new bug.

@sonatard
sonatard commented Jul 8, 2016 edited

Hello!!
I have a question about sharing values between middlewares.

I think we'll be able to share values between middlewares in the following code by go 1.7.

func (a *authHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        user := GetUserFromRequestCookie(r)
        ctx := r.Context()
        ctx = context.WithValue(ctx, "user", user)

        r = r.WithContext(ctx)

        a.next.ServeHTTP(w, r) // next handler is ArticleHandle
}

func (a *ArticleHandle) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    ctx := r.Context() 
    user := ctx.Value("user").(*User)
}

But r.WithContext(ctx) exec shallow copy for goroutine. I need not shallow copy for only sharing values. It is slow. right? I want to set context to Request. But now it cannot.

func (a *authHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
        user := UserFromSession(r)
        ctx := r.Context()
        ctx = context.WithValue(ctx, "user", user)

        // r = r.WithContext(ctx)
        r.SetContext(ctx)

        a.next.ServeHTTP(w, r)
}

func (a *ArticleHandle) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    ctx := r.Context() 
    user := ctx.Value("user").(*User)
}

Please tell me best way sharing values between middlewares using only net/http and context. I think we should not implement context wrapper like 3rd party framework impl for only sharing values.

@davecheney
Contributor

@sona-tar We don't the issue tracker to ask questions. Please see https://golang.org/wiki/Questions for good places to ask. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment