Fix races in txnBuf

Fix races in txnBuf

Runs clean with `-cpu=2,4 -race` now. It's also a lot clearer to follow the
logic now. No more 'siblingLock'. At the time that each recursive transaction
starts it locks (in the normal way!). It then keeps a reference to the parent
layer with the locks disabled for its internal usage.

This refactor makes it so that all (interacting) operations at a given 'layer'
in a transaction are mutually exclusive. Previously only nested transactions
would be mutually exclusive.

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

Committed: https://github.com/luci/gae/commit/6a79257cdd6c624326829b679d4b548ef1123942

[dnj, 2015-11-11 16:08:28]

lgtm w/ comments/nits, agreed that this is cleaner.

It's a little weird for the inner state to use the outer instances' locks, but
with the many-to-one relationship here it's fine.

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/ds_txn...
filter/txnBuf/ds_txn.go:26: haveLock bool
WDYT about replacing this with a "sync.Locker" that, in the "haveLock" case, is
no-op?

type noopLocker struct {}
func (noopLocker) Lock() {}
func (noopLocker) Unlock() {}

This will remove all of the conditionals and just make it Lock/Unlock.

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/ds_txn...
filter/txnBuf/ds_txn.go:85: if !d.haveLock {
Can we make this block an inline function and use "defer" here?

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/state....
filter/txnBuf/state.go:483: func (t *txnBufState) canApply(s *txnBufState) error
{
canApplyLocked?

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/state....
filter/txnBuf/state.go:495: func (t *txnBufState) commit(s *txnBufState) {
commitLocked?

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/txnbuf...
File filter/txnBuf/txnbuf_test.go (right):

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/txnbuf...
filter/txnBuf/txnbuf_test.go:158: FocusConvey("Good", func() {
Switch back to regular Convey.

https://chromiumcodereview.appspot.com/1434873003/diff/1/filter/txnBuf/txnbuf...
filter/txnBuf/txnbuf_test.go:190: FocusConvey("multi-level read/write", func() {
Switch back to regular Convey.

[iannucci, 2015-11-11 18:06:40]

ptal

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/ds_txn.go#new...
filter/txnBuf/ds_txn.go:26: haveLock bool
On 2015/11/11 at 16:08:28, dnj wrote:
> WDYT about replacing this with a "sync.Locker" that, in the "haveLock" case,
is no-op?
> 
> type noopLocker struct {}
> func (noopLocker) Lock() {}
> func (noopLocker) Unlock() {}
> 
> This will remove all of the conditionals and just make it Lock/Unlock.

I agree it would be cleaner (fewer conditionals), but I think the condition
serves an important role to call attention to a WeirdThing(tm) happening here.

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/ds_txn.go#new...
filter/txnBuf/ds_txn.go:85: if !d.haveLock {
On 2015/11/11 at 16:08:28, dnj wrote:
> Can we make this block an inline function and use "defer" here?

yep. I guess the `dup` call could crash.

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/state.go#newc...
filter/txnBuf/state.go:483: func (t *txnBufState) canApply(s *txnBufState) error
{
On 2015/11/11 at 16:08:28, dnj wrote:
> canApplyLocked?

done

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/state.go#newc...
filter/txnBuf/state.go:495: func (t *txnBufState) commit(s *txnBufState) {
On 2015/11/11 at 16:08:28, dnj wrote:
> commitLocked?

done

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/txnbuf_test.go
File filter/txnBuf/txnbuf_test.go (right):

https://codereview.chromium.org/1434873003/diff/1/filter/txnBuf/txnbuf_test.g...
filter/txnBuf/txnbuf_test.go:190: FocusConvey("multi-level read/write", func() {
On 2015/11/11 at 16:08:28, dnj wrote:
> Switch back to regular Convey.

oops, done (for both)

[iannucci, 2015-11-11 18:50:29]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-11-11 18:50:30]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1434873003/#ps20001
(title: "fix stuff")

[commit-bot: I haz the power, 2015-11-11 18:50:40]

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

[commit-bot: I haz the power, 2015-11-11 18:54:19]

Committed patchset #2 (id:20001) as
https://github.com/luci/gae/commit/6a79257cdd6c624326829b679d4b548ef1123942