Add Context cancellation to clock.

Add Context cancellation to clock.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/86e9094387fea7e47efe06fba090fe3118f66650

[iannucci, 2016-02-09 20:41:10]

https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go
File common/clock/cancelSleep.go (right):

https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go...
common/clock/cancelSleep.go:18: func CancelSleep(c context.Context, d
time.Duration) error {
(discussed offline) let's just make normal Sleep (and After) take a context, on
the assumption it's never safe to sleep while ignoring the context.Done anyway
:)

[Vadim Sh., 2016-02-09 21:11:37]

https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go
File common/clock/cancelSleep.go (right):

https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go...
common/clock/cancelSleep.go:18: func CancelSleep(c context.Context, d
time.Duration) error {
On 2016/02/09 20:41:10, iannucci wrote:
> (discussed offline) let's just make normal Sleep (and After) take a context,
on
> the assumption it's never safe to sleep while ignoring the context.Done anyway
> :)

I'd prefer to keep CancelSleep. clock package is mirroring time API pretty
closely, making Sleep cancelable will break this and introduce some WTF moments.

Also there's an issue for not paying (performance wise) for what you don't need.
I can imagine Sleep to be used in some simple lock-free busy loops (for
example). Plugging in context cancelling machinery there can be a big
performance impact.

[dnj (Google), 2016-02-10 01:24:14]

On 2016/02/09 21:11:37, Vadim Sh. wrote:
> https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go
> File common/clock/cancelSleep.go (right):
> 
>
https://codereview.chromium.org/1679023005/diff/1/common/clock/cancelSleep.go...
> common/clock/cancelSleep.go:18: func CancelSleep(c context.Context, d
> time.Duration) error {
> On 2016/02/09 20:41:10, iannucci wrote:
> > (discussed offline) let's just make normal Sleep (and After) take a context,
> on
> > the assumption it's never safe to sleep while ignoring the context.Done
anyway
> > :)
> 
> I'd prefer to keep CancelSleep. clock package is mirroring time API pretty
> closely, making Sleep cancelable will break this and introduce some WTF
moments.
> 
> Also there's an issue for not paying (performance wise) for what you don't
need.
> I can imagine Sleep to be used in some simple lock-free busy loops (for
> example). Plugging in context cancelling machinery there can be a big
> performance impact.

ATM the implementation that I have working changes:
1) After, NewTimer, and Sleep all take Contexts now, and have paths for error
values.
2) If those Contexts are canceled or expired, they will terminate the
sleep/timer with the Context's error code.
3) Contexts can have timer tags added to them so that timers can be
differentiated from each other in tests.

Performance-wise, the impacts are:
(1) Modifies the API in a not horrible way. One extra parameter to a handful of
calls doesn't, in practice, really hurt things. The return values are also very
similar.
(2) causes all sleeps to spawn a monitoring goroutine. Sleep is implemented
using After, and After will select between time.After and the Context, whichever
finishes first. 
(3) adds a Context derivation code to each additional tag in the chain. However,
Context traversal only happens via test timer.

This does have the API-affecting change that sleep is no longer absolute, and
can terminate prematurely. In practice, this probably will only be noticeable in
cases where both a Context and a timer are selected on. The remedy is basically
to have it select on just the timer, since that implicitly selects on the
Context. Sleep being canceled on Context cancellation doesn't feel too weird to
me, mostly b/c I'm used to things that take Context arguments being subject to
the Context.

The extra monitoirng goroutine per sleep is an expense is a performance
consideration. One option that comes to mind is to offer an "uninterruptable
system clock" implementation that really does fall through to straight system.
This can be used for micro-sleeps.

WDYT?

[dnj, 2016-02-10 03:09:18]

Description was changed from

==========
Add CancelSleep method to clock package.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
Add Context cancellation to clock. Cleanup.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=
==========

[dnj (Google), 2016-02-10 03:19:03]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2016-02-10 03:19:04]

PTAL, updated based on discussion. Grew a bit, but for the better IMO. Have
annotations.

https://codereview.chromium.org/1679023005/diff/20001/common/clock/clockconte...
File common/clock/clockcontext_test.go (right):

https://codereview.chromium.org/1679023005/diff/20001/common/clock/clockconte...
common/clock/clockcontext_test.go:17: type manualClock struct {
This test used to lean on system "time" clock. Since it's implementing the
Context in a high level, using an instrumented clock provides a much nicer
testing space.

https://codereview.chromium.org/1679023005/diff/20001/common/clock/systemcloc...
File common/clock/systemclock.go (right):

https://codereview.chromium.org/1679023005/diff/20001/common/clock/systemcloc...
common/clock/systemclock.go:46: go func() {
I implemented this separately from systemtimer b/c it is slightly more
lightweight. Not opposed to making this a NewTimer / <-GetC() call like system
and testclock do.

https://codereview.chromium.org/1679023005/diff/20001/common/gcloud/pubsub/ac...
File common/gcloud/pubsub/ackbuffer/ackbuffer.go (right):

https://codereview.chromium.org/1679023005/diff/20001/common/gcloud/pubsub/ac...
common/gcloud/pubsub/ackbuffer/ackbuffer.go:94: go func() {
Sorry to heap this in, but it's somewhat bound to the clock changes (removing
"meter" package). This is fairly straightforward, though. Batches up and dump by
size or timer threshold.

https://codereview.chromium.org/1679023005/diff/20001/common/meter/meter.go
File common/meter/meter.go (left):

https://codereview.chromium.org/1679023005/diff/20001/common/meter/meter.go#o...
common/meter/meter.go:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
Lots of LOC removed here. This was originally shared by 4 components, but each
has moved away from it except "ackbuffer". I hit some race conditions that are
inherent to the test cases, and I realized that the use/test cases themselves
are grossly overcomplicated for the single remaining use. I transposed the
important stuff into ackbuffer and decided to kill this guy off.

https://codereview.chromium.org/1679023005/diff/20001/common/parallel/semapho...
File common/parallel/semaphore.go (right):

https://codereview.chromium.org/1679023005/diff/20001/common/parallel/semapho...
common/parallel/semaphore.go:31: func (s Semaphore) TakeAll() {
This is really useful. Before, one would have to use a WaitGroup in parallel to
a Semaphore to Join against it. Now it's a single command! Needed b/c ackbuffer
was using WaitGroup.

[dnj (Google), 2016-02-10 03:19:36]

Description was changed from

==========
Add Context cancellation to clock. Cleanup.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
Add Context cancellation to clock.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=
==========

[dnj (Google), 2016-02-10 03:40:09]

https://codereview.chromium.org/1679023005/diff/40001/appengine/memlock/memlo...
File appengine/memlock/memlock.go (right):

https://codereview.chromium.org/1679023005/diff/40001/appengine/memlock/memlo...
appengine/memlock/memlock.go:156: if (<-clock.After(subCtx, delay)).Err != nil {
Nice, eh?

https://codereview.chromium.org/1679023005/diff/40001/common/clock/testclock/...
File common/clock/testclock/testclock.go (right):

https://codereview.chromium.org/1679023005/diff/40001/common/clock/testclock/...
common/clock/testclock/testclock.go:148: // If we finished naturally, but our
Context is Done, prefer the Context
memlock tests were depending on this preferring a Context error over an
already-timed-out timer.

[Vadim Sh., 2016-02-10 19:39:16]

Adding optional functionality that forced on you mandatory into low level
functions like Sleep is a way for software to become bloated and slow, see Java.

I won't be arguing. I'll just had to remember not to use common/clock in
performance critical sections and just come up with something adhoc instead.

[iannucci, 2016-02-10 22:30:28]

looks pretty good to me!

Some thingies tho

https://codereview.chromium.org/1679023005/diff/60001/common/clock/clock.go
File common/clock/clock.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/clock.go#n...
common/clock/clock.go:26: // naturally, it will return nil.
CancelSleep comment

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
File common/clock/systemtimer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:16: ctx context.Context
base? ctx?

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:22: timerStoppedC chan struct{}
comments

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:39: select {
I'd use t.ctx.Err() instead

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:72: }
also Err()

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:92: func (t *systemTimer) reset() {
let's inline this

https://codereview.chromium.org/1679023005/diff/60001/common/clock/tags.go
File common/clock/tags.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/tags.go#ne...
common/clock/tags.go:13: const clockTagKey clockTagKeyType = 0
var clockTagKey = &"holds a []string for tagging clock stuff"

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testclock.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:155: break
merrrr

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:157: callback(now, err)
call with TimerResult?

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:183: // has an error.
s/error/be all closed up in here.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testclock_test.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock_test.go:17: // to exist and not be a test timer
type.
move into testTimer_test.go.test.tester.go

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testtimer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer.go:66: t.cancelFunc = nil
Stop (https://golang.org/pkg/time/#Timer.Stop) shouldn't signal or close the
channel

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer.go:96: func HasTags(t clock.Timer, tags
...string) bool {
HasTags(t clock.Timer, first string, rest ...string) bool

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testtimer_test.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer_test.go:146: So(GetTags(t), ShouldBeNil)
add test for block on channel and concurrent Stop() (maybe on system clock too?)

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer_test.go:200: Convey(`A context with flags {"A",
"B"}`, t, func() {
s/flags/tags

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go
File common/clock/timer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:16: // GetC returns the underlying timer's channel.
doc behavior with Reset and Stop

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:32: // TimerResult is the result fo a timer operation.
fo'

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:36: // terminated, Err will be non-nil.
time will be the time when the result was generated

https://codereview.chromium.org/1679023005/diff/60001/common/gcloud/pubsub/ac...
File common/gcloud/pubsub/ackbuffer/ackbuffer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/gcloud/pubsub/ac...
common/gcloud/pubsub/ackbuffer/ackbuffer.go:103: buf = buf[:0]
also reset timer

[dnj (Google), 2016-02-11 01:26:55]

Thanks for the review!

RE vadimsh@: I hear 'ya. I originally wanted this to be low-level, but found
myself adding the same boilerplate "expire or context cancel" to each use of the
timer. I think that taking a context as the first parameter is a fairly
unambiguous way of saying that the method is subject to the Context's
cancellation, and happens to be what I and iannucci@ want basically every time.

For performant code (e.g., nanosecond loops), it will definitely be noticeable
overhead. If you hit this case yeah, either use time.Sleep or, if you want to be
able to mock things, add a raw system timer implementation that is trivial and
explicitly not subject to Context. WDYT?

https://codereview.chromium.org/1679023005/diff/60001/common/clock/clock.go
File common/clock/clock.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/clock.go#n...
common/clock/clock.go:26: // naturally, it will return nil.
On 2016/02/10 22:30:28, iannucci wrote:
> CancelSleep comment

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
File common/clock/systemtimer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:16: ctx context.Context
On 2016/02/10 22:30:28, iannucci wrote:
> base? ctx?

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:22: timerStoppedC chan struct{}
On 2016/02/10 22:30:28, iannucci wrote:
> comments

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:39: select {
On 2016/02/10 22:30:28, iannucci wrote:
> I'd use t.ctx.Err() instead

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:72: }
On 2016/02/10 22:30:28, iannucci wrote:
> also Err()

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/systemtime...
common/clock/systemtimer.go:92: func (t *systemTimer) reset() {
On 2016/02/10 22:30:28, iannucci wrote:
> let's inline this

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/tags.go
File common/clock/tags.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/tags.go#ne...
common/clock/tags.go:13: const clockTagKey clockTagKeyType = 0
On 2016/02/10 22:30:28, iannucci wrote:
> var clockTagKey = &"holds a []string for tagging clock stuff"

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testclock.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:155: break
On 2016/02/10 22:30:28, iannucci wrote:
> merrrr

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:157: callback(now, err)
On 2016/02/10 22:30:28, iannucci wrote:
> call with TimerResult?

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock.go:183: // has an error.
On 2016/02/10 22:30:28, iannucci wrote:
> s/error/be all closed up in here.

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testclock_test.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testclock_test.go:17: // to exist and not be a test timer
type.
On 2016/02/10 22:30:28, iannucci wrote:
> move into testTimer_test.go.test.tester.go

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testtimer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer.go:66: t.cancelFunc = nil
On 2016/02/10 22:30:28, iannucci wrote:
> Stop (https://golang.org/pkg/time/#Timer.Stop) shouldn't signal or close the
> channel

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer.go:96: func HasTags(t clock.Timer, tags
...string) bool {
On 2016/02/10 22:30:28, iannucci wrote:
> HasTags(t clock.Timer, first string, rest ...string) bool

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
File common/clock/testclock/testtimer_test.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer_test.go:146: So(GetTags(t), ShouldBeNil)
On 2016/02/10 22:30:28, iannucci wrote:
> add test for block on channel and concurrent Stop() (maybe on system clock
too?)

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/testclock/...
common/clock/testclock/testtimer_test.go:200: Convey(`A context with flags {"A",
"B"}`, t, func() {
On 2016/02/10 22:30:28, iannucci wrote:
> s/flags/tags

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go
File common/clock/timer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:16: // GetC returns the underlying timer's channel.
On 2016/02/10 22:30:28, iannucci wrote:
> doc behavior with Reset and Stop

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:32: // TimerResult is the result fo a timer operation.
On 2016/02/10 22:30:28, iannucci wrote:
> fo'

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/clock/timer.go#n...
common/clock/timer.go:36: // terminated, Err will be non-nil.
On 2016/02/10 22:30:28, iannucci wrote:
> time will be the time when the result was generated

Done.

https://codereview.chromium.org/1679023005/diff/60001/common/gcloud/pubsub/ac...
File common/gcloud/pubsub/ackbuffer/ackbuffer.go (right):

https://codereview.chromium.org/1679023005/diff/60001/common/gcloud/pubsub/ac...
common/gcloud/pubsub/ackbuffer/ackbuffer.go:103: buf = buf[:0]
On 2016/02/10 22:30:28, iannucci wrote:
> also reset timer

Done.

[iannucci, 2016-02-11 02:15:33]

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go
File common/clock/clock.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go#n...
common/clock/clock.go:27: Sleep(context.Context, time.Duration) error
Vadim suggests (and I agree) that this should probably be called something now,
to call it out from time.Sleep, which doesn't return an error. Suggestions:
  * SleepErr
  * CancelableSleep
  * ???

Probably the same for After as well.

The concern is code like:

for <condition> {
  clock.Sleep(time.Second) # <- doesn't check err
  // hot loop! hot loop!
}

[iannucci, 2016-02-11 02:16:35]

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go
File common/clock/clock.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go#n...
common/clock/clock.go:27: Sleep(context.Context, time.Duration) error
On 2016/02/11 02:15:33, iannucci wrote:
> Vadim suggests (and I agree) that this should probably be called something
now,
> to call it out from time.Sleep, which doesn't return an error. Suggestions:
>   * SleepErr
>   * CancelableSleep
>   * ???
> 
> Probably the same for After as well.
> 
> The concern is code like:
> 
> for <condition> {
>   clock.Sleep(time.Second) # <- doesn't check err
>   // hot loop! hot loop!
> }

Which, I realize, sounds silly considering that you already had it named
CancelSleep :). But the difference is that it would be the only Sleep-like call.

[iannucci, 2016-02-11 02:24:04]

https://codereview.chromium.org/1679023005/diff/80001/common/clock/systemcloc...
File common/clock/systemclock_test.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/systemcloc...
common/clock/systemclock_test.go:24: const timeBase = 60 * time.Millisecond
Note that runtime.Gosched may also be pertinent here, which explicitly allows go
to run something else.

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go
File common/clock/tags.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go#ne...
common/clock/tags.go:11: var clockTagKey = &[]string{"clock-tag"}
so I think this is what I was thinking of:

https://github.com/golang/appengine/blob/54bf9150c922186bfc45a00bf9dfcb91a506...

does that seem ok?

[dnj (Google), 2016-02-11 17:22:37]

Updated w/ some thoughts.

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go
File common/clock/clock.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/clock.go#n...
common/clock/clock.go:27: Sleep(context.Context, time.Duration) error
On 2016/02/11 02:16:35, iannucci wrote:
> On 2016/02/11 02:15:33, iannucci wrote:
> > Vadim suggests (and I agree) that this should probably be called something
> now,
> > to call it out from time.Sleep, which doesn't return an error. Suggestions:
> >   * SleepErr
> >   * CancelableSleep
> >   * ???
> > 
> > Probably the same for After as well.
> > 
> > The concern is code like:
> > 
> > for <condition> {
> >   clock.Sleep(time.Second) # <- doesn't check err
> >   // hot loop! hot loop!
> > }
> 
> Which, I realize, sounds silly considering that you already had it named
> CancelSleep :). But the difference is that it would be the only Sleep-like
call.

My initial reaction is that I don't like the idea of changing the name, (a)
because SleepErr and the like is ugly and (b) because the error generally does
not need to be checked.

Something that cancels Contexts and also loops should be responding to the
Context's cancellation somewhere in the loop, else we're in a different sort of
trouble. Code that doesn't cancel Contexts won't be affected. I can't really
think of a case where a loop over Sleep in code that cancels its Contexts isn't
a bug.

Just my initial thoughts, am happy to discuss.

https://codereview.chromium.org/1679023005/diff/80001/common/clock/systemcloc...
File common/clock/systemclock_test.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/systemcloc...
common/clock/systemclock_test.go:24: const timeBase = 60 * time.Millisecond
On 2016/02/11 02:24:04, iannucci wrote:
> Note that runtime.Gosched may also be pertinent here, which explicitly allows
go
> to run something else.

I've used it in a some of the test code. The problem is that while it yields, it
doesn't guarantee goroutine order or that other gorotuines will run, so it's
flaky at best.

Using actual timing is important for specifically this suite of tests because
I'm specifically testing how the real system clock would work. There are one or
two cases that could be flaky (e.g., they may or may not assert their test
condition) because of this, but their role is mostly to assert conformance, and
the flake will result in a success either way (so not breaking flake), either
passes and asserts condition or passes and does not.

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go
File common/clock/tags.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go#ne...
common/clock/tags.go:11: var clockTagKey = &[]string{"clock-tag"}
On 2016/02/11 02:24:04, iannucci wrote:
> so I think this is what I was thinking of:
> 
>
https://github.com/golang/appengine/blob/54bf9150c922186bfc45a00bf9dfcb91a506...
> 
> does that seem ok?

OK, done.

[iannucci, 2016-02-12 03:35:36]

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go
File common/clock/tags.go (right):

https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go#ne...
common/clock/tags.go:11: var clockTagKey = &[]string{"clock-tag"}
On 2016/02/11 17:22:37, dnj (Google) wrote:
> On 2016/02/11 02:24:04, iannucci wrote:
> > so I think this is what I was thinking of:
> > 
> >
>
https://github.com/golang/appengine/blob/54bf9150c922186bfc45a00bf9dfcb91a506...
> > 
> > does that seem ok?
> 
> OK, done.

OH REALLY!?

[dnj (Google), 2016-02-12 04:09:07]

On 2016/02/12 03:35:36, iannucci wrote:
> https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go
> File common/clock/tags.go (right):
> 
>
https://codereview.chromium.org/1679023005/diff/80001/common/clock/tags.go#ne...
> common/clock/tags.go:11: var clockTagKey = &[]string{"clock-tag"}
> On 2016/02/11 17:22:37, dnj (Google) wrote:
> > On 2016/02/11 02:24:04, iannucci wrote:
> > > so I think this is what I was thinking of:
> > > 
> > >
> >
>
https://github.com/golang/appengine/blob/54bf9150c922186bfc45a00bf9dfcb91a506...
> > > 
> > > does that seem ok?
> > 
> > OK, done.
> 
> OH REALLY!?

I suppose I should upload the patch. IS THAT WHAT YOU WANT?

[iannucci, 2016-02-12 04:17:36]

lgtm

I think the Sleep behavior discrepancy is OK. The function signature is
different than time's, and I think for luci-go applications, this is the correct
behavior (and probably infra and infra_internal too).

Lets keep an eye on it, but I expect, realistically, that it won't be an issue
(and I also expect that there's code that's Sleeping while holding a context,
but not actually checking the context for cancellation).

[dnj, 2016-02-13 00:42:32]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-02-13 00:42:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1679023005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1679023005/100001

[commit-bot: I haz the power, 2016-02-13 00:46:44]

Description was changed from

==========
Add Context cancellation to clock.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
Add Context cancellation to clock.

Add Context cancellation to the clock package methods. Effectively this
makes it so that Clock methods (Sleep, After, Timer) will terminate
early if their supplied Context is canceled.

Update other packages to accommodate this. This update includes:
- Update "ackbuffer" to not use the "meter" package.
- Add a "TakeAll" method to parallel's Semaphore, which takes all
  instances of the semaphore. This is effectively a Join on the Semaphore.
- Remove the "meter" package. This is easier than fixing its tests, and
  (with "ackbuffer" out of the way) all of the uses that originally
  inspired it have switched to more custom methods.

R=iannucci@chromium.org, vadimsh@chromium.org
BUG=

Committed:
https://github.com/luci/luci-go/commit/86e9094387fea7e47efe06fba090fe3118f66650
==========

[commit-bot: I haz the power, 2016-02-13 00:46:45]

Committed patchset #6 (id:100001) as
https://github.com/luci/luci-go/commit/86e9094387fea7e47efe06fba090fe3118f66650