storage: Remove IsOpen/IsClosed method and reword

CHANGELOG.md

@@ -30,6 +30,8 @@
    change allows log implementations which don't need revisions.
  * Removed `Rollback` methods from storage interfaces, as `Close` is enough to
    cover the use-case.
+ * Removed the unused `IsOpen` and `IsClosed` methods from transaction
+   interfaces.
  * TODO(pavelkalinnikov): More changes are coming, and will be added here.
 
 ## v1.3.13

server/log_rpc_server_test.go

@@ -635,7 +635,6 @@ func TestGetProofByHashErrors(t *testing.T) {
 				tx.EXPECT().GetLeavesByHash(gomock.Any(), [][]byte{leafHash1}, false).Return(nil, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(&trillian.SignedLogRoot{}, errors.New("SLR"))
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getInclusionProofByHashRequest7,
 			errStr: "SLR",
@@ -648,7 +647,6 @@ func TestGetProofByHashErrors(t *testing.T) {
 				tx.EXPECT().GetLeavesByHash(gomock.Any(), [][]byte{leafHash1}, false).Return(nil, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(corruptLogRoot, nil)
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getInclusionProofByHashRequest7,
 			errStr: "not read current log root",
@@ -862,7 +860,6 @@ func TestGetProofByIndex(t *testing.T) {
 				s.EXPECT().SnapshotForTree(gomock.Any(), cmpMatcher{tree1}).Return(tx, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(&trillian.SignedLogRoot{}, errors.New("SLR"))
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getInclusionProofByIndexRequest7,
 			errStr: "SLR",
@@ -874,7 +871,6 @@ func TestGetProofByIndex(t *testing.T) {
 				s.EXPECT().SnapshotForTree(gomock.Any(), cmpMatcher{tree1}).Return(tx, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(corruptLogRoot, nil)
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getInclusionProofByIndexRequest7,
 			errStr: "not read current log root",
@@ -1031,7 +1027,6 @@ func TestGetEntryAndProof(t *testing.T) {
 				s.EXPECT().SnapshotForTree(gomock.Any(), cmpMatcher{tree1}).Return(tx, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(signedRoot1, errors.New("SLR"))
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getEntryAndProofRequest17,
 			errStr: "SLR",
@@ -1043,7 +1038,6 @@ func TestGetEntryAndProof(t *testing.T) {
 				s.EXPECT().SnapshotForTree(gomock.Any(), cmpMatcher{tree1}).Return(tx, nil)
 				tx.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(corruptLogRoot, nil)
 				tx.EXPECT().Close().Return(nil)
-				tx.EXPECT().IsOpen().AnyTimes().Return(false)
 			},
 			req:    &getEntryAndProofRequest17,
 			errStr: "not read current log root",
@@ -1573,7 +1567,6 @@ func TestInitLog(t *testing.T) {
 				} else {
 					mockTX.EXPECT().LatestSignedLogRoot(gomock.Any()).Return(tc.slr, nil)
 				}
-				mockTX.EXPECT().IsOpen().AnyTimes().Return(false)
 				mockTX.EXPECT().Close().Return(nil)
 			}
 			if tc.wantInit {

storage/admin_storage.go

@@ -25,20 +25,13 @@ import (
 type ReadOnlyAdminTX interface {
 	AdminReader
 
-	// Commit applies the operations performed to the underlying storage, or
-	// returns an error.
-	// A commit must be performed before any reads from storage are
-	// considered consistent.
+	// Commit applies the operations performed to the underlying storage. It must
+	// be called before any reads from storage are considered consistent.
 	Commit() error
 
-	// IsClosed returns true if the transaction is closed.
-	// A transaction is closed when either Commit() or Close() are called.
-	IsClosed() bool
-
-	// Close rolls back the transaction if it's not yet closed.
-	// It's advisable to call "defer tx.Close()" after the creation of
-	// transaction to ensure that it's always rolled back if not explicitly
-	// committed.
+	// Close rolls back the transaction if it wasn't committed or closed
+	// previously. Resources are cleaned up regardless of the success, and the
+	// transaction should not be used after it.
 	Close() error
 }
 

storage/cloudspanner/admin.go

@@ -82,19 +82,19 @@ func reverseTreeTypeMap(m map[trillian.TreeType]spannerpb.TreeType) map[spannerp
 type adminTX struct {
 	client *spanner.Client
 
+	// mu guards tx, but it's only actively used for Commit/Close. In other
+	// scenarios we trust Spanner to blow up if you try to use a closed tx.
+	//
+	// Note that, if tx is a spanner.SnapshotTransaction, it'll be set to nil
+	// when adminTX is closed.
+	mu sync.RWMutex
+
 	// tx is either spanner.ReadOnlyTransaction or spanner.ReadWriteTransaction,
 	// according to the role adminTX is meant to fill.
-	// If tx is a snapshot transaction it'll be set to nil when adminTX is
-	// closed to avoid reuse.
+	//
+	// If tx is a snapshot transaction it'll be set to nil when adminTX is closed
+	// to avoid reuse.
 	tx spanRead
-
-	// mu guards closed, but it's only actively used for
-	// Commit/Close. In other scenarios we trust Spanner to blow up
-	// if you try to use a closed tx.
-	// Note that, if tx is a spanner.SnapshotTransaction, it'll be set to
-	// nil when adminTX is closed.
-	mu     sync.RWMutex
-	closed bool
 }
 
 // adminStorage implements storage.AdminStorage.
@@ -137,13 +137,6 @@ func (t *adminTX) Commit() error {
 	return t.Close()
 }
 
-// IsClosed implements ReadOnlyAdminTX.IsClosed.
-func (t *adminTX) IsClosed() bool {
-	t.mu.RLock()
-	defer t.mu.RUnlock()
-	return t.closed
-}
-
 // Close implements ReadOnlyAdminTX.Close.
 func (t *adminTX) Close() error {
 	t.mu.Lock()

storage/cloudspanner/tree_storage.go

@@ -279,37 +279,18 @@ func (t *treeTX) Commit(ctx context.Context) error {
 // Close aborts any operations perfomed on the underlying Spanner transaction.
 // On return from the call, this transaction will be in a closed state.
 func (t *treeTX) Close() error {
-	if !t.IsOpen() {
-		return nil
-	}
-
 	t.mu.Lock()
-	defer func() {
-		t.stx = nil
-		t.mu.Unlock()
-	}()
-
+	defer t.mu.Unlock()
 	if t.stx == nil {
 		return ErrTransactionClosed
 	}
-
 	if stx, ok := t.stx.(*spanner.ReadOnlyTransaction); ok {
 		glog.V(1).Infof("Closed snapshot %p", stx)
 		stx.Close()
 	}
-
 	return nil
 }
 
-// IsOpen returns true iff neither Commit nor Close have been called.
-// If this function returns false, further operations may not be attempted on
-// this transaction object.
-func (t *treeTX) IsOpen() bool {
-	t.mu.RLock()
-	defer t.mu.RUnlock()
-	return t.stx != nil
-}
-
 // readRevision returns the tree revision at which the currently visible (taking
 // into account read-staleness) STH was stored.
 func (t *treeTX) readRevision(ctx context.Context) (int64, error) {

storage/log_storage.go

@@ -26,11 +26,13 @@ import (
 type ReadOnlyLogTX interface {
 	LogMetadata
 
-	// Commit ensures the data read by the TX is consistent in the database. Only after Commit the
-	// data read should be regarded as valid.
+	// Commit applies the operations performed to the underlying storage. It must
+	// be called before any reads from storage are considered consistent.
 	Commit(context.Context) error
 
-	// Close attempts to rollback the TX if it's open, it's a noop otherwise.
+	// Close rolls back the transaction if it wasn't committed or closed
+	// previously. Resources are cleaned up regardless of the success, and the
+	// transaction should not be used after it.
 	Close() error
 }
 

storage/memory/admin_storage.go

@@ -57,31 +57,22 @@ func (s *memoryAdminStorage) CheckDatabaseAccessible(ctx context.Context) error
 
 type adminTX struct {
 	ms *TreeStorage
-	// mu guards reads/writes on closed, which happen only on
-	// Commit/IsClosed/Close methods.
-	// We don't check closed on *all* methods (apart from the ones above),
-	// as we trust tx to keep tabs on its state (and consequently fail to do
-	// queries after closed).
+
+	// mu guards reads/writes on closed, which happen on Commit/Close methods.
+	//
+	// We don't check closed on methods apart from the ones above, as we trust tx
+	// to keep tabs on its state, and hence fail to do queries after closed.
 	mu     sync.RWMutex
 	closed bool
 }
 
 func (t *adminTX) Commit() error {
-	// TODO(al): The admin implementation isn't transactional
-	t.mu.Lock()
-	defer t.mu.Unlock()
-	t.closed = true
-	return nil
-}
-
-func (t *adminTX) IsClosed() bool {
-	t.mu.RLock()
-	defer t.mu.RUnlock()
-	return t.closed
+	// TODO(al): The admin implementation isn't transactional.
+	return t.Close()
 }
 
 func (t *adminTX) Close() error {
-	// TODO(al): The admin implementation isn't transactional
+	// TODO(al): The admin implementation isn't transactional.
 	t.mu.Lock()
 	defer t.mu.Unlock()
 	t.closed = true

storage/memory/tree_storage.go

@@ -260,14 +260,10 @@ func (t *treeTX) Commit(ctx context.Context) error {
 }
 
 func (t *treeTX) Close() error {
-	if !t.IsOpen() {
+	if t.closed {
 		return nil
 	}
 	defer t.unlock()
 	t.closed = true
 	return nil
 }
-
-func (t *treeTX) IsOpen() bool {
-	return !t.closed
-}

storage/mysql/admin_storage.go

@@ -101,11 +101,10 @@ func (s *mysqlAdminStorage) CheckDatabaseAccessible(ctx context.Context) error {
 type adminTX struct {
 	tx *sql.Tx
 
-	// mu guards *direct* reads/writes on closed, which happen only on
-	// Commit/IsClosed/Close methods.
-	// We don't check closed on *all* methods (apart from the ones above),
-	// as we trust tx to keep tabs on its state (and consequently fail to do
-	// queries after closed).
+	// mu guards reads/writes on closed, which happen on Commit/Close methods.
+	//
+	// We don't check closed on methods apart from the ones above, as we trust tx
+	// to keep tabs on its state, and hence fail to do queries after closed.
 	mu     sync.RWMutex
 	closed bool
 }
@@ -117,18 +116,12 @@ func (t *adminTX) Commit() error {
 	return t.tx.Commit()
 }
 
-func (t *adminTX) IsClosed() bool {
-	t.mu.RLock()
-	defer t.mu.RUnlock()
-	return t.closed
-}
-
 func (t *adminTX) Close() error {
-	if t.IsClosed() {
-		return nil
-	}
 	t.mu.Lock()
 	defer t.mu.Unlock()
+	if t.closed {
+		return nil
+	}
 	t.closed = true
 	return t.tx.Rollback()
 }

storage/mysql/tree_storage.go

@@ -399,20 +399,12 @@ func (t *treeTX) rollbackInternal() error {
 func (t *treeTX) Close() error {
 	t.mu.Lock()
 	defer t.mu.Unlock()
-
-	if !t.closed {
-		err := t.rollbackInternal()
-		if err != nil {
-			glog.Warningf("Rollback error on Close(): %v", err)
-		}
-		return err
+	if t.closed {
+		return nil
 	}
-	return nil
-}
-
-func (t *treeTX) IsOpen() bool {
-	t.mu.Lock()
-	defer t.mu.Unlock()
-
-	return !t.closed
+	err := t.rollbackInternal()
+	if err != nil {
+		glog.Warningf("Rollback error on Close(): %v", err)
+	}
+	return err
 }