code review 99330045: go.net/context: define the Context type, which supports...

go.net/context: define the Context type, which supports propagating
deadlines, cancellation, and other values across APIs and between
processes.

[Sameer Ajmani, 2014-05-16 18:29:34]

Hello rsc@golang.org, crawshaw@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.net

[gobot, 2014-05-16 18:33:44]

Replacing golang-dev with golang-codereviews.

To the author of this CL: 

If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
instead.

If you did not name golang-dev explicitly and it was still added to the CL,
it means your working copy of the repo has a stale codereview.cfg
(or lib/codereview/codereview.cfg).
Please run 'hg sync' to update your client to the most recent codereview.cfg.
If the most recent codereview.cfg has defaultcc set to golang-dev instead of
golang-codereviews, please send a CL correcting it.

Thanks very much.

[rsc, 2014-05-16 18:40:59]

https://codereview.appspot.com/99330045/diff/40001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode1
context/context.go:1: // Package context defines the Context type, which carries
deadlines, cancellation
missing copyright notices
golang.org/doc/contribute.html

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode325
context/context.go:325: panic("BUG: context canceled with a nil err")
please make these real error messages.

panic("context: internal error: missing cancel error")

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode329
context/context.go:329: panic("BUG: context canceled but constructed with nil
done channel")
panic("context: internal error: missing done channel")

https://codereview.appspot.com/99330045/diff/40001/context/withtimeout_test.go
File context/withtimeout_test.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/withtimeout_test.g...
context/withtimeout_test.go:14: sleep(ctx, 200*time.Millisecond)
does this really format correctly? it seems unlikely to me.
i think you have to inline sleep here or the example will be incomplete when
shown.

[Sameer Ajmani, 2014-05-16 18:58:22]

https://codereview.appspot.com/99330045/diff/40001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode1
context/context.go:1: // Package context defines the Context type, which carries
deadlines, cancellation
On 2014/05/16 18:40:59, rsc wrote:
> missing copyright notices
> golang.org/doc/contribute.html

Done.

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode325
context/context.go:325: panic("BUG: context canceled with a nil err")
On 2014/05/16 18:40:59, rsc wrote:
> please make these real error messages.
> 
> panic("context: internal error: missing cancel error")

Done.

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode329
context/context.go:329: panic("BUG: context canceled but constructed with nil
done channel")
On 2014/05/16 18:40:59, rsc wrote:
> panic("context: internal error: missing done channel")

Done.

https://codereview.appspot.com/99330045/diff/40001/context/withtimeout_test.go
File context/withtimeout_test.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/withtimeout_test.g...
context/withtimeout_test.go:14: sleep(ctx, 200*time.Millisecond)
On 2014/05/16 18:40:59, rsc wrote:
> does this really format correctly? it seems unlikely to me.
> i think you have to inline sleep here or the example will be incomplete when
> shown.

Inlined.  Will test with local godoc.

[crawshaw, 2014-05-16 19:14:16]

There needs to be some story for how this relates to a request-scoped context,
i.e. http://www.gorillatoolkit.org/pkg/context

It could be as simple as a note in the package doc that some future
requestcontext package is planned, that given some common request scopes (e.g.
*http.Request, or an RPC call object), returns a context.Context.

https://codereview.appspot.com/99330045/diff/40001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode61
context/context.go:61: var keys = make(map[string]*Key)
Why use *Key throughout instead of Key?

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode168
context/context.go:168: if c == nil {
As you explicitly encourage people in the documentation to not use nil for a
context, why check for it here?

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode192
context/context.go:192: // TODO returns the Background context.  Code should use
context.TODO when it's
Perhaps // TODO returns an ambient background context.

There could be some debugging value in giving the ctx type a String method,
which can print "context.Background" or "context.TODO" or a more helpful message
about the context being done, or information about its deadline.

[Sameer Ajmani, 2014-05-16 19:49:57]

https://codereview.appspot.com/99330045/diff/40001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode61
context/context.go:61: var keys = make(map[string]*Key)
On 2014/05/16 19:14:16, crawshaw wrote:
> Why use *Key throughout instead of Key?

Done.

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode168
context/context.go:168: if c == nil {
On 2014/05/16 19:14:15, crawshaw wrote:
> As you explicitly encourage people in the documentation to not use nil for a
> context, why check for it here?

Done.

https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode192
context/context.go:192: // TODO returns the Background context.  Code should use
context.TODO when it's
On 2014/05/16 19:14:15, crawshaw wrote:
> Perhaps // TODO returns an ambient background context.
> 
> There could be some debugging value in giving the ctx type a String method,
> which can print "context.Background" or "context.TODO" or a more helpful
message
> about the context being done, or information about its deadline.

Done.

[Sameer Ajmani, 2014-05-16 19:55:55]

On 2014/05/16 19:14:16, crawshaw wrote:
> There needs to be some story for how this relates to a request-scoped context,
> i.e. http://www.gorillatoolkit.org/pkg/context
> 
> It could be as simple as a note in the package doc that some future
> requestcontext package is planned, that given some common request scopes (e.g.
> *http.Request, or an RPC call object), returns a context.Context.
> 
> https://codereview.appspot.com/99330045/diff/40001/context/context.go
> File context/context.go (right):
> 
>
https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode61
> context/context.go:61: var keys = make(map[string]*Key)
> Why use *Key throughout instead of Key?
> 
>
https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode168
> context/context.go:168: if c == nil {
> As you explicitly encourage people in the documentation to not use nil for a
> context, why check for it here?
> 
>
https://codereview.appspot.com/99330045/diff/40001/context/context.go#newcode192
> context/context.go:192: // TODO returns the Background context.  Code should
use
> context.TODO when it's
> Perhaps // TODO returns an ambient background context.
> 
> There could be some debugging value in giving the ctx type a String method,
> which can print "context.Background" or "context.TODO" or a more helpful
message
> about the context being done, or information about its deadline.

Added a note in the package docs about *http.Request and similar.

[crawshaw, 2014-05-16 20:10:28]

LGTM

Given we have some experience with this code elsewhere, I'm happy to see it
checked in now so we can try it out in other packages.

[Sameer Ajmani, 2014-05-16 20:11:31]

*** Submitted as
https://code.google.com/p/go/source/detail?r=9c33002573cb&repo=net ***

go.net/context: define the Context type, which supports propagating
deadlines, cancellation, and other values across APIs and between
processes.

LGTM=crawshaw
R=rsc, crawshaw
CC=bradfitz, golang-codereviews
https://codereview.appspot.com/99330045