A simple memcache lock for appengine.

go/src/infra/gae/libs/memlock/memlock.go

Index: go/src/infra/gae/libs/memlock/memlock.go
diff --git a/go/src/infra/gae/libs/memlock/memlock.go b/go/src/infra/gae/libs/memlock/memlock.go
new file mode 100644
index 0000000000000000000000000000000000000000..1e3e8911595bfa6e40a8c50aecd335b24af3474a
--- /dev/null
+++ b/go/src/infra/gae/libs/memlock/memlock.go
@@ -0,0 +1,175 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// +build appengine
+
+package memlock
+
+import (
+	"bytes"
+	"crypto/rand"
+	"sync/atomic"
+	"time"
+
+	"appengine/memcache"
+
+	"infra/gae/libs/context"
+	"infra/libs/logging"
+)
+
+type checkOp string
+
+var delay = time.Second

[M-A Ruel, 2015/03/30 12:52:31]

You can use a const.

+
+const (
+	release checkOp = "release"

[M-A Ruel, 2015/03/30 12:52:31]

releaseOp
refreshOp

+	refresh         = "refresh"
+)
+
+// memcacheLockTime is the expiration time of the memcache entry. If the lock
+// is correctly released, then it will be released before this time.
+var memcacheLockTime = 16 * time.Second

[M-A Ruel, 2015/03/30 12:52:31]

You can use a const.

16s is hell of a long time

+
+type Context interface {

[M-A Ruel, 2015/03/30 12:52:31]

If it's exported, document it.

+	logging.Logger
+	context.MCSingleReadWriter
+}
+
+// MemcacheLock allows multiple appengine handlers to coordinate best-effort
+// mutual execution via memcache. "best-effort" here means "best-effort"...
+// memcache is not reliable. However, colliding on memcache is a lot cheaper
+// than, for example, colliding with datastore transactions.
+//
+// Construct a new MemcacheLock with the New() method.
+type MemcacheLock struct {
+	ctx      Context
+	key      string
+	clientID []byte
+	held     bool
+}
+
+// New creates a MemcacheLock. `key` is the memcache key to use. Clients locking
+// the same data must use the same key. clientID is the unique identifier for
+// this client (lock-holder). If it's empty then New() will generate a random

[M-A Ruel, 2015/03/30 12:52:31]

Why generate a random one? What's the use case?

+// one.
+//
+// Using a deterministid clientID (like the taskqueue task name, for example) has
+// the benefit that on an error the re-tried taskqueue handler may be able to
+// re-obtain the lock, assuming it wasn't released properly.
+func New(c Context, key, clientID string) (*MemcacheLock, error) {
+	clientIDbytes := []byte(clientID)
+	if len(clientIDbytes) == 0 {
+		clientIDbytes = make([]byte, 32)
+		_, err := rand.Read(clientIDbytes)
+		if err != nil {
+			return nil, err

[M-A Ruel, 2015/03/30 12:52:30]

In this case specifically (using crypto/rand) I think it's fine to panic.

Note that I'm generally against using panic but here it's just noise. If
crypto/rand is empty, you have bigger problems

+		}
+		c.Debugf("memlock: generated clientId: %v", clientIDbytes)
+	}
+	return &MemcacheLock{
+		ctx:      c,
+		key:      "memlock:" + key,
+		clientID: clientIDbytes,
+	}, nil
+}
+
+// TryWithLock attempts to obtains the lock once, and then invokes f if
+// sucessful. The `check` function can be used within f to see if the lock is
+// still held. This function returns true iff the lock was acquired and f was
+// invoked.
+func (m *MemcacheLock) TryWithLock(f func(check func() bool)) bool {

[M-A Ruel, 2015/03/30 12:52:31]

The main issue with that is that you force the caller to use a polling loop and
cannot use a select() flow anymore. It depends, sometimes a channel is more
appropriate, sometimes a loop is better. In practice in GAE, using a channel is
likely more often the right design.

+	if m.held {
+		m.logf(m.ctx.Errorf, "re-entering memlock!")
+		panic("re-entering memlock!")
+	}
+	m.held = true

[M-A Ruel, 2015/03/30 12:52:30]

To double check; MemcacheLock is not thread safe.

+	defer func() { m.held = false }()
+
+	err := m.ctx.Add(&memcache.Item{
+		Key: m.key, Expiration: memcacheLockTime, Value: m.clientID})
+	if err != nil {
+		if err != memcache.ErrNotStored {
+			m.logf(m.ctx.Warningf, "error adding: %s", err)
+		}
+		if !m.checkAnd(refresh) {
+			return false
+		}
+	}
+
+	stopChan := make(chan struct{})
+	stoppedChan := make(chan struct{})
+	var held uint32 = 1
+
+	check := func() bool {
+		return atomic.LoadUint32(&held) == 1
+	}
+	defer func() {
+		close(stopChan)
+		<-stoppedChan
+	}()
+
+	go func() {
+		defer close(stoppedChan)
+
+	checkLoop:
+		for {
+			select {
+			case <-stopChan:
+				break checkLoop

[M-A Ruel, 2015/03/30 12:52:31]

use return instead, this removes need for goto

+			case <-time.After(delay):
+			}
+			if !m.checkAnd(refresh) {
+				atomic.StoreUint32(&held, 0)
+				m.logf(m.ctx.Warningf, "lost lock: %s", err)
+				break

[M-A Ruel, 2015/03/30 12:52:31]

remove

+			}
+		}
+
+		m.checkAnd(release)

[M-A Ruel, 2015/03/30 12:52:30]

why not in defer?

+		atomic.StoreUint32(&held, 0)
+	}()
+
+	f(check)
+	return true
+}
+
+func (m *MemcacheLock) logf(f func(string, ...interface{}), fmt string, args ...interface{}) {

[M-A Ruel, 2015/03/30 12:52:30]

I personally do not see much value in this, as it obfuscate the code to save a
few (8) copy paste.

+	args = append([]interface{}{m.key, string(m.clientID)}, args...)
+	f("memlock(%s:%q): "+fmt, args...)
+}
+
+func (m *MemcacheLock) checkAnd(op checkOp) bool {

[M-A Ruel, 2015/03/30 12:52:31]

It's more like compareAndExchange(), which would be clearer to me.

+	itm, err := m.ctx.Get(m.key)
+	if err != nil {
+		m.logf(m.ctx.Warningf, "error getting: %s", err)
+		return false
+	}
+
+	if len(itm.Value) != 0 && !bytes.Equal(itm.Value, m.clientID) {
+		m.logf(m.ctx.Infof, "lock owned by %q", string(itm.Value))

[M-A Ruel, 2015/03/30 12:52:31]

This will become very verbose eventually.

+		return false
+	}
+
+	if op == refresh {
+		itm.Value = m.clientID
+		itm.Expiration = memcacheLockTime
+	} else {
+		if len(itm.Value) == 0 {
+			// it's already unlocked, no need to CAS
+			m.logf(m.ctx.Infof, "lock already released")
+			return true
+		}
+		itm.Value = []byte{}
+		itm.Expiration = delay
+	}
+
+	err = m.ctx.CompareAndSwap(itm)

[M-A Ruel, 2015/03/30 12:52:31]

I don't like that it's doing 2 memcache operations on normal state, it should
aim at doing the fast path first.

+	if err != nil {
+		m.logf(m.ctx.Warningf, "failed to %s lock: %q", op, err)
+		return false
+	}
+
+	m.logf(m.ctx.Infof, "%sed lock", op)

[M-A Ruel, 2015/03/30 12:52:31]

Remove.

+	return true
+}