backend: skip *bolt.DB.Size call when nil #6662

gyuho · 2016-10-18T04:02:31Z

batchTx.commit(false) from newBatchTx
batchTx.commit(true) from stopping backend
batchTx.commit(false) from inflight mvcc Hash call

would panic because *bolt.Tx.Commit in batchTx.commit
initializes *bolt.Tx.db and *bolt.Tx.meta as nil,
and subsequent *bolt.Tx.Size() call refers to this nil
pointer (panic).

Fix etcd-io/etcdlabs#30.

/cc @xiang90

xiang90 · 2016-10-18T16:48:53Z

mvcc/backend/batch_tx_test.go

+	defer cleanup(b, tmpPath)
+
+	// 2. call batchTx.commit(true)
+	b.batchTx.CommitAndStop()


this seems to be wrong. CommitAndStop only can be called by backend once it stopped. no other operation can be issued to backend after that. if you want to prevent bad caller that calls backend.X() after backend gets stopped, you should protect the backend side.

I think ongoing Hash call before stopping would still trigger this panic.
I might be wrong. I will read mvcc, backend again to investigate more.

Thanks!

gyuho · 2016-10-20T03:06:44Z

@xiang90 Added more test cases which all panic without this patch.

To make it clear, this is only useful for projects embedding etcd servers. If a project embeds etcd server, this panic would shut down the whole application. And there's no way to know in advance because etcd server stop method returns after calling backend.Close. https://github.com/coreos/etcd/blob/master/etcdserver/server.go#L667-L672

heyitsanthony · 2016-10-20T18:45:30Z

Why would an in-flight Hash break it but not an in-flight Get or Put?

gyuho · 2016-10-20T18:49:29Z

@heyitsanthony I think same thing can happen in other in-flight RPC calls like get, put.
Hash just happens to be my use case in using embedded etcd servers. I periodically call
Hash to get states of etcd servers, and there seems to be no easy to prevent or shut down
in-flight Hash calls after stopping backend that internally happens in etcdserver.EtcdServer.run.

heyitsanthony · 2016-10-20T19:07:51Z

mvcc/backend/batch_tx.go

@@ -159,6 +161,12 @@ func (t *batchTx) commit(stop bool) {
 	var err error
 	// commit the last tx
 	if t.tx != nil {
+		if t.stopped {


dropping a commit on the floor like this without an error and acting like it worked is kind of scary; what if the commit is important?

agree. let me rethink on this. thanks.

gyuho · 2016-10-20T20:38:29Z

@xiang90 @heyitsanthony How about checking if db is nil without adding stopped field?

--- a/mvcc/backend/batch_tx.go
+++ b/mvcc/backend/batch_tx.go
@@ -162,7 +162,9 @@ func (t *batchTx) commit(stop bool) {
                if t.pending == 0 && !stop {
                        t.backend.mu.RLock()
                        defer t.backend.mu.RUnlock()
-                       atomic.StoreInt64(&t.backend.size, t.tx.Size())
+                       if t.tx.DB() != nil {
+                               atomic.StoreInt64(&t.backend.size, t.tx.Size())
+                       }
                        return
                }
                start := time.Now()

heyitsanthony · 2016-10-21T17:44:52Z

integration/v3_maintenance_test.go

+
+// TestV3MaintenanceHash ensures concurrent Hash call to embedded EtcdServer
+// that is being stopped does not panic.
+func TestV3MaintenanceHash(t *testing.T) {


TestV3MaintenanceHashInflight?

heyitsanthony · 2016-10-21T17:46:18Z

mvcc/backend/batch_tx.go

@@ -162,7 +162,9 @@ func (t *batchTx) commit(stop bool) {
 		if t.pending == 0 && !stop {
 			t.backend.mu.RLock()
 			defer t.backend.mu.RUnlock()
-			atomic.StoreInt64(&t.backend.size, t.tx.Size())
+			if t.tx.DB() != nil {


heyitsanthony · 2016-10-21T17:48:03Z

lgtm after fixing nits

Fix etcd-io/etcdlabs#30.