Simplify memlock and make it less racy.

Simplify memlock and make it less racy.

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

Committed: https://chromium.googlesource.com/infra/infra/+/d7aa9c069daa8362432e22a2b8c68a07236da2cf

[iannucci, 2015-07-17 07:44:37]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-17 07:44:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233413002/1

[commit-bot: I haz the power, 2015-07-17 07:46:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-17 07:46:55]

Dry run: This issue passed the CQ dry run.

[dnj, 2015-07-17 18:01:35]

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
File go/src/infra/gae/libs/memlock/memlock.go (right):

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
go/src/infra/gae/libs/memlock/memlock.go:59: // ErrEmptyClientID.
Document that the context that "f" is called with can be cancelled to release
the lock.

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
go/src/infra/gae/libs/memlock/memlock.go:131: subCtx, cancelFunc :=
context.WithCancel(ctx)
Main concern using contexts here is that if the parent cancels its context for
some other reason, that will also cancel this context and thus release the lock.
Is this desired?

Alternatively, you can go back to the "check()" function method and use a
channel instead of 'atomic' to do the check, replacing "subCtx/cancelFunc" with:

cancelC := make(chan struct{})
cancelOnce := sync.Once{}
cancelFunc := func() {
  cancelOnce.Do(func() {
    close(cancelC)
  })
}

[iannucci, 2015-07-17 18:08:25]

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
File go/src/infra/gae/libs/memlock/memlock.go (right):

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
go/src/infra/gae/libs/memlock/memlock.go:59: // ErrEmptyClientID.
On 2015/07/17 18:01:35, dnj wrote:
> Document that the context that "f" is called with can be cancelled to release
> the lock.

I think you're misunderstanding how the cancelation works (f doesn't have a
handle to cancel the context, just TryWithLock), but I've updating the
documentation anyhow.

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
go/src/infra/gae/libs/memlock/memlock.go:131: subCtx, cancelFunc :=
context.WithCancel(ctx)
On 2015/07/17 18:01:35, dnj wrote:
> Main concern using contexts here is that if the parent cancels its context for
> some other reason, that will also cancel this context and thus release the
lock.
> Is this desired?

Yes

> 
> Alternatively, you can go back to the "check()" function method and use a
> channel instead of 'atomic' to do the check, replacing "subCtx/cancelFunc"
with:
> 
> cancelC := make(chan struct{})
> cancelOnce := sync.Once{}
> cancelFunc := func() {
>   cancelOnce.Do(func() {
>     close(cancelC)
>   })
> }

[dnj, 2015-07-17 18:10:29]

Okay, lgtm then!

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
File go/src/infra/gae/libs/memlock/memlock.go (right):

https://codereview.chromium.org/1233413002/diff/1/go/src/infra/gae/libs/memlo...
go/src/infra/gae/libs/memlock/memlock.go:59: // ErrEmptyClientID.
On 2015/07/17 18:08:25, iannucci wrote:
> On 2015/07/17 18:01:35, dnj wrote:
> > Document that the context that "f" is called with can be cancelled to
release
> > the lock.
> 
> I think you're misunderstanding how the cancelation works (f doesn't have a
> handle to cancel the context, just TryWithLock), but I've updating the
> documentation anyhow.

Ah bad comment on my part, I was mostly suggesting you document that the
context's "Done" channel is used to detect cancellation.

[iannucci, 2015-07-17 18:16:50]

Vadim did you want to take a peek too?

[Vadim Sh., 2015-07-17 19:44:12]

On 2015/07/17 18:16:50, iannucci wrote:
> Vadim did you want to take a peek too?

nah, I trust Dan's judgment :)

[iannucci, 2015-07-17 20:03:12]

On 2015/07/17 19:44:12, Vadim Sh. wrote:
> On 2015/07/17 18:16:50, iannucci wrote:
> > Vadim did you want to take a peek too?
> 
> nah, I trust Dan's judgment :)

:)

[iannucci, 2015-07-17 20:03:15]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-17 20:03:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233413002/40001

[commit-bot: I haz the power, 2015-07-17 20:05:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/d7aa9c069daa8362432e22a2b8c68...