Fix memory corruption bug in impl/memory

impl/memory/gkvlite_tracing_utils.go

Index: impl/memory/gkvlite_tracing_utils.go
diff --git a/impl/memory/gkvlite_tracing_utils.go b/impl/memory/gkvlite_tracing_utils.go
new file mode 100644
index 0000000000000000000000000000000000000000..e766e432d5eb17cad0474dd681c86e390219b681
--- /dev/null
+++ b/impl/memory/gkvlite_tracing_utils.go
@@ -0,0 +1,188 @@
+// 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.
+
+package memory
+
+import (
+	"encoding/hex"
+	"flag"
+	"fmt"
+	"os"
+	"runtime"
+	"sync"
+	"sync/atomic"
+
+	"github.com/luci/gkvlite"
+)
+
+var logMemCollectionFolder = flag.String(

[dnj, 2016/04/22 06:09:44]

Note this will be compiled into every binary. It may be worth splitting this off
as a sub-package. Just a thought.

[iannucci, 2016/04/22 06:56:25]

I thought about that, but I think it'll be worse to have to change the source
and recompile to debug a crash (if there is one).

+	"luci.gae.gkvlite_trace_folder", "",
+	"Set to a folder path to enable debugging traces to be dumped there. Set to '-' to dump to stdout.")
+var logMemCollectionOnce sync.Once
+var logMemCounter uint32
+var logMemNameKey = "holds a string indicating the GKVLiteDebuggingTraceName"
+var stdoutLock sync.Mutex
+
+func wrapTracingMemStore(store memStore) memStore {
+	var writer traceWriter
+	logNum := atomic.AddUint32(&logMemCounter, 1) - 1
+	collName := fmt.Sprintf("coll%d", logNum)
+
+	if *logMemCollectionFolder == "-" {
+		writer = func(format string, a ...interface{}) {
+			stdoutLock.Lock()
+			defer stdoutLock.Unlock()
+			fmt.Printf(format+"\n", a...)
+		}
+	} else {
+		logMemCollectionOnce.Do(func() {
+			fmt.Fprintln(os.Stderr, "REMOVING LOG COLLECTION FOLDER:", *logMemCollectionFolder)

[dnj, 2016/04/22 06:09:44]

The possibility of me doing something stupid here like setting this to "/tmp" or
"$HOME" makes me really scared about the RemoveAll part. WDYT about just using
the folder. If the user wants to clean it, they can "rm" its contents before
running the code pretty easily:

$ rm $PLACE; go test -v luci.gae.gkvlite_trace_folder=$PLACE

That or create a "luci-gae"-prefixed tempdir within the folder that is specified
and write there. This has the added benefit of letting the user easily compare
different runs, and cleanup is just "rm -rf $PLACE/luci-gae*"

[iannucci, 2016/04/22 06:56:24]

Good point, changed to use a tempdir at the designated folder.

+			if err := os.RemoveAll(*logMemCollectionFolder); !(err == os.ErrNotExist || err == nil) {
+				panic(err)
+			}
+			if err := os.MkdirAll(*logMemCollectionFolder, 0755); err != nil {

[dnj, 2016/04/22 06:09:44]

I would say the user should ensure that the directory exists, and it's an error
if it doesn't.

[iannucci, 2016/04/22 06:56:25]

Acknowledged.

+				panic(err)
+			}
+		})
+
+		lck := sync.Mutex{}
+		fname := fmt.Sprintf("%s/%d.trace", *logMemCollectionFolder, logNum)

[dnj, 2016/04/22 06:09:44]

Use filepath.Join: https://golang.org/pkg/path/filepath/#Join

Also should probably abspath *logMemCollectionFolder to remove any problems if
the user changes CWD an the path is relative. You are a mere package after all
:)

[iannucci, 2016/04/22 06:56:25]

Done.

+		fil, err := os.Create(fname)
+		if err != nil {
+			panic(err)
+		}
+		writer = func(format string, a ...interface{}) {
+			lck.Lock()
+			defer lck.Unlock()
+			fmt.Fprintf(fil, format+"\n", a...)
+		}
+		runtime.SetFinalizer(&writer, func(_ *traceWriter) { fil.Close() })
+	}
+	writer("%s := newMemStore()", collName)
+	return &tracingMemStoreImpl{store, writer, collName, 0, false}
+}
+
+type traceWriter func(format string, a ...interface{})
+
+type tracingMemStoreImpl struct {
+	i memStore
+	w traceWriter
+
+	collName string
+	// for the mutable store, this is a counter that increments for every
+	// Snapshot, and for snapshots, this is the number of the snapshot.
+	snapNum uint32
+	isSnap  bool
+}
+
+var _ memStore = (*tracingMemStoreImpl)(nil)
+
+func (t *tracingMemStoreImpl) ImATestingSnapshot() {}
+
+func (t *tracingMemStoreImpl) colWriter(action, name string) traceWriter {
+	ident := t.ident()
+	hexname := hex.EncodeToString([]byte(name))
+	writer := func(format string, a ...interface{}) {
+		if format[0] == '/' { // comment

[dnj, 2016/04/22 06:09:44]

Might as well just do a strings.HasPrefix("//") to be clear here.

[iannucci, 2016/04/22 06:56:25]

Done.

+			t.w(format, a...)
+		} else {
+			t.w(fmt.Sprintf("%s_%s%s", ident, hexname, format), a...)
+		}
+	}
+	writer(" := %s.%s(%q)", ident, action, name)
+	return writer
+}
+
+func (t *tracingMemStoreImpl) ident() string {
+	if t.isSnap {
+		return fmt.Sprintf("%s_snap%d", t.collName, t.snapNum)
+	}
+	return t.collName
+}
+
+func (t *tracingMemStoreImpl) GetCollection(name string) memCollection {
+	coll := t.i.GetCollection(name)
+	if coll == nil {
+		t.w("// %s.GetCollection(%q) -> nil", t.ident(), name)
+		return nil
+	}
+	writer := t.colWriter("GetCollection", name)
+	return &tracingMemCollectionImpl{coll, writer, 0}
+}
+
+func (t *tracingMemStoreImpl) GetCollectionNames() []string {
+	t.w("%s.GetCollectionNames()", t.ident())
+	return t.i.GetCollectionNames()
+}
+
+func (t *tracingMemStoreImpl) GetOrCreateCollection(name string) memCollection {
+	writer := t.colWriter("GetOrCreateCollection", name)
+	return &tracingMemCollectionImpl{t.i.GetOrCreateCollection(name), writer, 0}
+}
+
+func (t *tracingMemStoreImpl) Snapshot() memStore {
+	snap := t.i.Snapshot()
+	if snap == t.i {
+		t.w("// %s.Snapshot() -> self", t.ident())
+		return t
+	}
+	ret := &tracingMemStoreImpl{snap, t.w, t.collName, t.snapNum, true}
+	t.w("%s := %s.Snapshot()", ret.ident(), t.ident())
+	t.snapNum++

[dnj, 2016/04/22 06:09:44]

Non-atomic? Is Snapshot not goroutine-safe? I suppose not, since gkvlite needs
locks. Just checking though.

If not, just use an "int" here instead of a uint32. This is a thing I think we
settled on a few months back.

[iannucci, 2016/04/22 06:56:24]

not safe, snapshot already needs a lock.
uint is the correct type to use here, since we don't use this for subtraction,
and negative values will cause invalid trace file content. Changed to uint.

+	return ret
+}
+
+func (t *tracingMemStoreImpl) IsReadOnly() bool {
+	return t.i.IsReadOnly()
+}
+
+type tracingMemCollectionImpl struct {
+	i           memCollection
+	w           traceWriter
+	visitNumber uint32
+}
+
+var _ memCollection = (*tracingMemCollectionImpl)(nil)
+
+func (t *tracingMemCollectionImpl) Name() string {
+	return t.i.Name()
+}
+
+func (t *tracingMemCollectionImpl) Delete(k []byte) bool {
+	t.w(".Delete(%#v)", k)
+	return t.i.Delete(k)
+}
+
+func (t *tracingMemCollectionImpl) Get(k []byte) []byte {
+	t.w(".Get(%#v)", k)
+	return t.i.Get(k)
+}
+
+func (t *tracingMemCollectionImpl) GetTotals() (numItems, numBytes uint64) {
+	t.w(".GetTotals()")
+	return t.i.GetTotals()
+}
+
+func (t *tracingMemCollectionImpl) MinItem(withValue bool) *gkvlite.Item {
+	t.w(".MinItem(%t)", withValue)
+	return t.i.MinItem(withValue)
+}
+
+func (t *tracingMemCollectionImpl) Set(k, v []byte) {
+	t.w(".Set(%#v, %#v)", k, v)
+	t.i.Set(k, v)
+}
+
+func (t *tracingMemCollectionImpl) VisitItemsAscend(target []byte, withValue bool, visitor gkvlite.ItemVisitor) {
+	vnum := t.visitNumber

[dnj, 2016/04/22 06:09:44]

Same, non-atomic check? If not, just use an "int" here instead of a uint32. This
is a thing I think we settled on a few months back.

[iannucci, 2016/04/22 06:56:24]

changed to uint

+	t.visitNumber++
+
+	t.w(".VisitItemsAscend(%#v, %t, func(i *gkvlite.Item) bool{ return true }) // BEGIN VisitItemsAscend(%d)", target, withValue, vnum)
+	defer t.w("// END VisitItemsAscend(%d)", vnum)
+	t.i.VisitItemsAscend(target, withValue, visitor)
+}
+
+func (t *tracingMemCollectionImpl) IsReadOnly() bool {
+	return t.i.IsReadOnly()
+}