[ARVADOS] updated: 1.3.0-2722-gff22ba71a

[Git user]

Summary of changes:
 lib/controller/localdb/db.go      | 16 ++++++---
 lib/controller/localdb/db_test.go | 68 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 6 deletions(-)

       via  ff22ba71afe839832943099cc1fe273197c45ec7 (commit)
       via  34316951a4e4f8439940a7eda5f1b044565f072b (commit)
      from  2c5417221843491727e4e5505012fc115e3bc7b0 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit ff22ba71afe839832943099cc1fe273197c45ec7
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Tue Jun 30 17:01:54 2020 -0400

    16534: Test goroutine safety.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/lib/controller/localdb/db.go b/lib/controller/localdb/db.go
index a864e32d4..4f64e6352 100644
--- a/lib/controller/localdb/db.go
+++ b/lib/controller/localdb/db.go
@@ -78,9 +78,15 @@ type transactionFinishFunc func(*error)
 func starttx(ctx context.Context, getdb func(context.Context) (*sql.DB, error)) (context.Context, transactionFinishFunc) {
 	txn := &transaction{getdb: getdb}
 	return context.WithValue(ctx, contextKeyTransaction, txn), func(err *error) {
-		// Ensure another goroutine can't open a transaction
-		// during/after finishtx().
-		txn.setup.Do(func() {})
+		txn.setup.Do(func() {
+			// Using (*sync.Once)Do() prevents a future
+			// call to currenttx() from opening a
+			// transaction which would never get committed
+			// or rolled back. If currenttx() hasn't been
+			// called before now, future calls will return
+			// this error.
+			txn.err = errors.New("refusing to start a transaction after wrapped function already returned")
+		})
 		if txn.tx == nil {
 			// we never [successfully] started a transaction
 			return
diff --git a/lib/controller/localdb/db_test.go b/lib/controller/localdb/db_test.go
index 39ac524a6..5bab86c60 100644
--- a/lib/controller/localdb/db_test.go
+++ b/lib/controller/localdb/db_test.go
@@ -7,8 +7,12 @@ package localdb
 import (
 	"context"
 	"database/sql"
+	"sync"
+	"sync/atomic"
 
+	"git.arvados.org/arvados.git/lib/config"
 	"git.arvados.org/arvados.git/sdk/go/arvados"
+	"git.arvados.org/arvados.git/sdk/go/ctxlog"
 	_ "github.com/lib/pq"
 	check "gopkg.in/check.v1"
 )
@@ -30,3 +34,65 @@ func testctx(c *check.C, db *sql.DB) (ctx context.Context, rollback func()) {
 		c.Check(tx.Rollback(), check.IsNil)
 	}
 }
+
+var _ = check.Suite(&DatabaseSuite{})
+
+type DatabaseSuite struct{}
+
+func (*DatabaseSuite) TestTransactionContext(c *check.C) {
+	cfg, err := config.NewLoader(nil, ctxlog.TestLogger(c)).Load()
+	c.Assert(err, check.IsNil)
+	cluster, err := cfg.GetCluster("")
+	c.Assert(err, check.IsNil)
+
+	var getterCalled int64
+	getter := func(context.Context) (*sql.DB, error) {
+		atomic.AddInt64(&getterCalled, 1)
+		return testdb(c, cluster), nil
+	}
+	wrapper := WrapCallsInTransactions(getter)
+	wrappedFunc := wrapper(func(ctx context.Context, opts interface{}) (interface{}, error) {
+		txes := make([]*sql.Tx, 20)
+		var wg sync.WaitGroup
+		for i := range txes {
+			i := i
+			wg.Add(1)
+			go func() {
+				// Concurrent calls to currenttx(),
+				// with different children of the same
+				// parent context, will all return the
+				// same transaction.
+				defer wg.Done()
+				ctx, cancel := context.WithCancel(ctx)
+				defer cancel()
+				tx, err := currenttx(ctx)
+				c.Check(err, check.IsNil)
+				txes[i] = tx
+			}()
+		}
+		wg.Wait()
+		for i := range txes[1:] {
+			c.Check(txes[i], check.Equals, txes[i+1])
+		}
+		return true, nil
+	})
+
+	ok, err := wrappedFunc(context.Background(), "blah")
+	c.Check(ok, check.Equals, true)
+	c.Check(err, check.IsNil)
+	c.Check(getterCalled, check.Equals, int64(1))
+
+	// When a wrapped func returns without calling currenttx(),
+	// calling currenttx() later shouldn't start a new
+	// transaction.
+	var savedctx context.Context
+	ok, err = wrapper(func(ctx context.Context, opts interface{}) (interface{}, error) {
+		savedctx = ctx
+		return true, nil
+	})(context.Background(), "blah")
+	c.Check(ok, check.Equals, true)
+	c.Check(err, check.IsNil)
+	tx, err := currenttx(savedctx)
+	c.Check(tx, check.IsNil)
+	c.Check(err, check.NotNil)
+}

commit 34316951a4e4f8439940a7eda5f1b044565f072b
Author: Tom Clegg <tom at tomclegg.ca>
Date:   Tue Jun 30 16:29:02 2020 -0400

    16534: Rename Transaction -> ContextWithTransaction.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>

diff --git a/lib/controller/localdb/db.go b/lib/controller/localdb/db.go
index f9c5d19e2..a864e32d4 100644
--- a/lib/controller/localdb/db.go
+++ b/lib/controller/localdb/db.go
@@ -30,10 +30,10 @@ func WrapCallsInTransactions(getdb func(context.Context) (*sql.DB, error)) func(
 	}
 }
 
-// WithTransaction returns a child context in which the given
+// ContextWithTransaction returns a child context in which the given
 // transaction will be used by any localdb API call that needs one.
 // The caller is responsible for calling Commit or Rollback on tx.
-func Transaction(ctx context.Context, tx *sql.Tx) context.Context {
+func ContextWithTransaction(ctx context.Context, tx *sql.Tx) context.Context {
 	txn := &transaction{tx: tx}
 	txn.setup.Do(func() {})
 	return context.WithValue(ctx, contextKeyTransaction, txn)
diff --git a/lib/controller/localdb/db_test.go b/lib/controller/localdb/db_test.go
index 5187dfecd..39ac524a6 100644
--- a/lib/controller/localdb/db_test.go
+++ b/lib/controller/localdb/db_test.go
@@ -26,7 +26,7 @@ func testdb(c *check.C, cluster *arvados.Cluster) *sql.DB {
 func testctx(c *check.C, db *sql.DB) (ctx context.Context, rollback func()) {
 	tx, err := db.Begin()
 	c.Assert(err, check.IsNil)
-	return Transaction(context.Background(), tx), func() {
+	return ContextWithTransaction(context.Background(), tx), func() {
 		c.Check(tx.Rollback(), check.IsNil)
 	}
 }

-----------------------------------------------------------------------


hooks/post-receive
--