code review 116400043: go.net/context: split the implementations of Background...

go.net/context: split the implementations of Background, WithValue,
WithCancel, and WithTimeout to use different concrete types.  Update the
tests and documentation.

This change reduces the size of context structs, reduces the number of
allocations (see TestAllocs) and removes unnecessary pointers from the
heap, such as the timer field for non-timer contexts.

IMPORTANT: I've removed the code in these functions that handles nil Context parameters.  Passing a nil parent Context will now cause a panic.

[Sameer Ajmani, 2014-07-29 20:19:56]

Hello bcmills@google.com, crawshaw@golang.org (cc: adonovan,
golang-codereviews@googlegroups.com, rsc),

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

[bcmills, 2014-07-29 21:35:00]

context.go only so far.

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

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:145: type emptyCtx int
"type emptyCtx struct{}" to emphasize that the value isn't meaningful?

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:277: mu       sync.RWMutex
Does an RWMutex actually buy much here?

(It looks like the only read-locked segment is quite short, so it may be more
efficient to use a regular Mutex - the extra locking overhead and space for the
RWMutex might not outweigh the reduction in lock-contention.)

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:299: if c.done == nil {
Is this check really necessary?  The "close(c.done)" later should produce a
nearly-equally-clear panic.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:306: close(c.done)
Maybe swap this line and the next?  That way the Mutex lock in the Err method is
just an added measure of safety rather than necessary for correctness - it would
always be safe to read c.err as soon as c.done is closed.

(Or, consider dropping c.mu entirely in the Err method and perhaps adding a
race-detector annotation instead.)

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:377: if c.timer != nil {
This leaks the timer if the parent context is canceled before the Lock at 352
and the caller skips the cancel func (which they may reasonably do if they've
checked the Done channel).

I think the fix is to make the c.cancelCtx.cancel call before stopping the timer
and guard the assignment at 354 with a check for cancellation:

if c.err == nil {
  c.timer = [...]
}
return c, [...]

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:409: // A valueCtx carries a key-value pair.  It implements
Value for that key an
s/an/and/

[bcmills, 2014-07-29 22:14:55]

Test LGTM.

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

https://codereview.appspot.com/116400043/diff/40001/context/context_test.go#n...
context/context_test.go:112: // The parent's children should contain the two
cancelable children.
This test seems... unpleasantly implementation-detaily, but I guess that's
unavoidable.

The property we're testing for appears to be "the child's cancel method is
called synchronously when the parent is canceled".  Perhaps it would be cleaner
to test that by putting a canceler implementation on the bottom of the stack
rather than inspecting the maps at the top?

(That would also lead to smaller test changes if at some point we open up the
canceler interface or one like it for third-party packages to use.)

https://codereview.appspot.com/116400043/diff/40001/context/context_test.go#n...
context/context_test.go:352: limit:      1,
Nice!

[Sameer Ajmani, 2014-07-30 14:43:30]

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

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:145: type emptyCtx int
On 2014/07/29 21:35:00, bcmills wrote:
> "type emptyCtx struct{}" to emphasize that the value isn't meaningful?

The value is meaningful (see String).  Also, struct{} would cause background and
todo to be indistinguishable.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:277: mu       sync.RWMutex
On 2014/07/29 21:34:59, bcmills wrote:
> Does an RWMutex actually buy much here?
> 
> (It looks like the only read-locked segment is quite short, so it may be more
> efficient to use a regular Mutex - the extra locking overhead and space for
the
> RWMutex might not outweigh the reduction in lock-contention.)

Done.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:299: if c.done == nil {
On 2014/07/29 21:35:00, bcmills wrote:
> Is this check really necessary?  The "close(c.done)" later should produce a
> nearly-equally-clear panic.

Done.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:306: close(c.done)
On 2014/07/29 21:34:59, bcmills wrote:
> Maybe swap this line and the next?  That way the Mutex lock in the Err method
is
> just an added measure of safety rather than necessary for correctness - it
would
> always be safe to read c.err as soon as c.done is closed.
> 
> (Or, consider dropping c.mu entirely in the Err method and perhaps adding a
> race-detector annotation instead.)

Done.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:377: if c.timer != nil {
On 2014/07/29 21:35:00, bcmills wrote:
> This leaks the timer if the parent context is canceled before the Lock at 352
> and the caller skips the cancel func (which they may reasonably do if they've
> checked the Done channel).
> 
> I think the fix is to make the c.cancelCtx.cancel call before stopping the
timer
> and guard the assignment at 354 with a check for cancellation:
> 
> if c.err == nil {
>   c.timer = [...]
> }
> return c, [...]

Done.  Reordered the code in WithDeadline a bit and removed some TODOs.

https://codereview.appspot.com/116400043/diff/40001/context/context.go#newcod...
context/context.go:409: // A valueCtx carries a key-value pair.  It implements
Value for that key an
On 2014/07/29 21:35:00, bcmills wrote:
> s/an/and/

Done.

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

https://codereview.appspot.com/116400043/diff/40001/context/context_test.go#n...
context/context_test.go:112: // The parent's children should contain the two
cancelable children.
On 2014/07/29 22:14:55, bcmills wrote:
> This test seems... unpleasantly implementation-detaily, but I guess that's
> unavoidable.
> 
> The property we're testing for appears to be "the child's cancel method is
> called synchronously when the parent is canceled".  Perhaps it would be
cleaner
> to test that by putting a canceler implementation on the bottom of the stack
> rather than inspecting the maps at the top?
> 
> (That would also lead to smaller test changes if at some point we open up the
> canceler interface or one like it for third-party packages to use.)

I don't plan to open up canceler; let's cross that bridge if we ever get there.
I like this implementation-specific test.  This is a tightly-coupled package,
and I want to check that the coupling is correct.

[Sameer Ajmani, 2014-07-30 14:53:16]

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

https://codereview.appspot.com/116400043/diff/40001/context/context_test.go#n...
context/context_test.go:352: limit:      1,
On 2014/07/29 22:14:55, bcmills wrote:
> Nice!

It turns out some of these limits were too high, probably due to changes in Go's
escape analysis since the test was written.  But even so, this CL improves on
the tightened limits.

[bcmills, 2014-07-30 15:44:26]

LGTM.

https://codereview.appspot.com/116400043/diff/80001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/116400043/diff/80001/context/context.go#newcod...
context/context.go:347: propagateCancel(parent, c)
Moving the propagateCancel call to here makes it so that the sequence:

parent, cancel := context.WithCancel(context.Background())
cancel()
child = context.WithDeadline(parent, time.Now())

has child.Err() returning DeadlineExceeded instead of Canceled.

Is that intentional/acceptable?

[Sameer Ajmani, 2014-07-30 16:14:44]

https://codereview.appspot.com/116400043/diff/80001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/116400043/diff/80001/context/context.go#newcod...
context/context.go:347: propagateCancel(parent, c)
On 2014/07/30 15:44:26, bcmills wrote:
> Moving the propagateCancel call to here makes it so that the sequence:
> 
> parent, cancel := context.WithCancel(context.Background())
> cancel()
> child = context.WithDeadline(parent, time.Now())
> 
> has child.Err() returning DeadlineExceeded instead of Canceled.
> 
> Is that intentional/acceptable?

It probably doesn't matter, but I'll reverse it.

[Sameer Ajmani, 2014-07-30 16:15:01]

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

go.net/context: split the implementations of Background, WithValue,
WithCancel, and WithTimeout to use different concrete types.  Update the
tests and documentation.

This change reduces the size of context structs, reduces the number of
allocations (see TestAllocs) and removes unnecessary pointers from the
heap, such as the timer field for non-timer contexts.

IMPORTANT: I've removed the code in these functions that handles nil Context
parameters.  Passing a nil parent Context will now cause a panic.

LGTM=bcmills
R=bcmills, crawshaw
CC=adonovan, golang-codereviews, rsc
https://codereview.appspot.com/116400043

[bcmills, 2014-07-30 16:19:56]

https://codereview.appspot.com/116400043/diff/100001/context/context.go
File context/context.go (right):

https://codereview.appspot.com/116400043/diff/100001/context/context.go#newco...
context/context.go:345: c.cancel(false, DeadlineExceeded) // deadline has
already passed
With propagateCancel back before this call, I think we want
removeFromParent=true here to avoid a tiny leak.