diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b961596c45c..f791f3caec60 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -43,7 +43,7 @@ new engineers. - GNU Make (3.81+ is known to work) - CMake 3.1+ - Autoconf 2.68+ - - NodeJS 6.x and Yarn 1.0+ + - NodeJS 6.x and Yarn 1.7+ Note that at least 4GB of RAM is required to build from source and run tests. diff --git a/pkg/cli/backtrace.go b/pkg/cli/backtrace.go index f395a83b0e22..86f4aadfc2d0 100644 --- a/pkg/cli/backtrace.go +++ b/pkg/cli/backtrace.go @@ -93,7 +93,7 @@ func initBacktrace(logDir string, options ...stop.Option) *stop.Stopper { bcd.Register(tracer) // Hook log.Fatal*. - log.SetExitFunc(func(code int) { + log.SetExitFunc(false /* hideStack */, func(code int) { _ = bcd.Trace(tracer, fmt.Errorf("exit %d", code), nil) os.Exit(code) }) diff --git a/pkg/cmd/allocsim/main.go b/pkg/cmd/allocsim/main.go index 02756baa3a2b..37b8863981a4 100644 --- a/pkg/cmd/allocsim/main.go +++ b/pkg/cmd/allocsim/main.go @@ -522,7 +522,7 @@ func main() { a := newAllocSim(c) a.localities = localities - log.SetExitFunc(func(code int) { + log.SetExitFunc(false /* hideStack */, func(code int) { c.Close() os.Exit(code) }) diff --git a/pkg/cmd/zerosum/main.go b/pkg/cmd/zerosum/main.go index 64ec013469f0..d138749ea447 100644 --- a/pkg/cmd/zerosum/main.go +++ b/pkg/cmd/zerosum/main.go @@ -441,7 +441,7 @@ func main() { c := &localcluster.LocalCluster{Cluster: localcluster.New(cfg)} defer c.Close() - log.SetExitFunc(func(code int) { + log.SetExitFunc(false /* hideStack */, func(code int) { c.Close() os.Exit(code) }) diff --git a/pkg/roachpb/api.pb.go b/pkg/roachpb/api.pb.go index fa36510e8579..20db7f229091 100644 --- a/pkg/roachpb/api.pb.go +++ b/pkg/roachpb/api.pb.go @@ -374,7 +374,9 @@ const ( // Don't do anything special, just note that the intent was not found in the // response. QueryIntentRequest_DO_NOTHING QueryIntentRequest_IfMissingBehavior = 0 - // Return an IntentMissingError. + // Return an IntentMissingError. Special-cased to return a SERIALIZABLE + // retry error if a SERIALIZABLE transaction queries its own intent and + // finds it has been pushed. QueryIntentRequest_RETURN_ERROR QueryIntentRequest_IfMissingBehavior = 1 // Prevent the intent from ever being written in the future. If set as the // behavior, a response with found_intent=false implies that an intent will @@ -1254,9 +1256,12 @@ type QueryIntentRequest struct { // be committed by the provided transaction. If an intent is found at the // specified key, the intent is only considered a match if it has the same ID, // the same epoch, and a provisional commit timestamp that is equal to or less - // than that in the provided transaction. If the intent's timestamp is greater - // than that in the provided transaction, it would prevent the transaction - // from committing and is therefore not a match. + // than that in the provided transaction. For SERIALIZABLE transactions, if + // the intent's timestamp is greater than that in the provided transaction, it + // would prevent the transaction from committing and is therefore not a match. + // However, for SNAPSHOT transactions, if the intent's timestamp is greater + // than that in the provided transaction, it would not prevent the transaction + // from committing and therefore is a match. // // Additionally, the intent is only considered a match if its sequence number // is equal to or greater than the expected txn's sequence number. The diff --git a/pkg/roachpb/api.proto b/pkg/roachpb/api.proto index 59abbd0e9c22..5d07f3a126e0 100644 --- a/pkg/roachpb/api.proto +++ b/pkg/roachpb/api.proto @@ -749,9 +749,12 @@ message QueryIntentRequest { // be committed by the provided transaction. If an intent is found at the // specified key, the intent is only considered a match if it has the same ID, // the same epoch, and a provisional commit timestamp that is equal to or less - // than that in the provided transaction. If the intent's timestamp is greater - // than that in the provided transaction, it would prevent the transaction - // from committing and is therefore not a match. + // than that in the provided transaction. For SERIALIZABLE transactions, if + // the intent's timestamp is greater than that in the provided transaction, it + // would prevent the transaction from committing and is therefore not a match. + // However, for SNAPSHOT transactions, if the intent's timestamp is greater + // than that in the provided transaction, it would not prevent the transaction + // from committing and therefore is a match. // // Additionally, the intent is only considered a match if its sequence number // is equal to or greater than the expected txn's sequence number. The @@ -767,7 +770,9 @@ message QueryIntentRequest { // Don't do anything special, just note that the intent was not found in the // response. DO_NOTHING = 0; - // Return an IntentMissingError. + // Return an IntentMissingError. Special-cased to return a SERIALIZABLE + // retry error if a SERIALIZABLE transaction queries its own intent and + // finds it has been pushed. RETURN_ERROR = 1; // Prevent the intent from ever being written in the future. If set as the // behavior, a response with found_intent=false implies that an intent will diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index d0e05b0f5b6c..06204d3d600c 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -815,8 +815,8 @@ func TestPersistHLCUpperBound(t *testing.T) { defer leaktest.AfterTest(t)() var fatal bool - defer log.SetExitFunc(os.Exit) - log.SetExitFunc(func(r int) { + defer log.ResetExitFunc() + log.SetExitFunc(true /* hideStack */, func(r int) { defer log.Flush() if r != 0 { fatal = true diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go index cfffa13cb981..73e919c443b1 100644 --- a/pkg/server/version_cluster_test.go +++ b/pkg/server/version_cluster_test.go @@ -17,7 +17,6 @@ package server_test import ( "context" gosql "database/sql" - "os" "path/filepath" "strconv" "sync/atomic" @@ -320,8 +319,8 @@ func TestClusterVersionMixedVersionTooOld(t *testing.T) { exits := make(chan int, 100) - log.SetExitFunc(func(i int) { exits <- i }) - defer log.SetExitFunc(os.Exit) + log.SetExitFunc(true /* hideStack */, func(i int) { exits <- i }) + defer log.ResetExitFunc() // Three nodes at v1.1 and a fourth one at 1.0, but all operating at v1.0. versions := [][2]string{{"1.0", "1.1"}, {"1.0", "1.1"}, {"1.0", "1.1"}, {"1.0", "1.0"}} @@ -376,8 +375,8 @@ func TestClusterVersionMixedVersionTooNew(t *testing.T) { exits := make(chan int, 100) - log.SetExitFunc(func(i int) { exits <- i }) - defer log.SetExitFunc(os.Exit) + log.SetExitFunc(true /* hideStack */, func(i int) { exits <- i }) + defer log.ResetExitFunc() // Three nodes at v1.1 and a fourth one (started later) at 1.1-2 (and // incompatible with anything earlier). diff --git a/pkg/sqlmigrations/migrations_test.go b/pkg/sqlmigrations/migrations_test.go index b2141e813323..10bebfe02709 100644 --- a/pkg/sqlmigrations/migrations_test.go +++ b/pkg/sqlmigrations/migrations_test.go @@ -18,7 +18,6 @@ import ( "bytes" "context" "fmt" - "os" "strings" "testing" "time" @@ -335,8 +334,8 @@ func TestLeaseExpiration(t *testing.T) { defer func() { leaseRefreshInterval = oldLeaseRefreshInterval }() exitCalled := make(chan bool) - log.SetExitFunc(func(int) { exitCalled <- true }) - defer log.SetExitFunc(os.Exit) + log.SetExitFunc(true /* hideStack */, func(int) { exitCalled <- true }) + defer log.ResetExitFunc() // Disable stack traces to make the test output in teamcity less deceiving. defer log.DisableTracebacks()() diff --git a/pkg/storage/batcheval/cmd_query_intent.go b/pkg/storage/batcheval/cmd_query_intent.go index 21f40a7d3c0f..7f6a1468ff1d 100644 --- a/pkg/storage/batcheval/cmd_query_intent.go +++ b/pkg/storage/batcheval/cmd_query_intent.go @@ -38,6 +38,7 @@ func QueryIntent( ctx context.Context, batch engine.ReadWriter, cArgs CommandArgs, resp roachpb.Response, ) (result.Result, error) { args := cArgs.Args.(*roachpb.QueryIntentRequest) + h := cArgs.Header reply := resp.(*roachpb.QueryIntentResponse) // Snapshot transactions cannot be prevented using a QueryIntent command. @@ -65,7 +66,11 @@ func QueryIntent( return result.Result{}, err } + // Determine if the request is querying an intent in its own transaction. + ownTxn := h.Txn != nil && h.Txn.ID == args.Txn.ID + var curIntent *roachpb.Intent + var curIntentPushed bool switch len(intents) { case 0: reply.FoundIntent = false @@ -75,8 +80,27 @@ func QueryIntent( // comparison. reply.FoundIntent = (args.Txn.ID == curIntent.Txn.ID) && (args.Txn.Epoch == curIntent.Txn.Epoch) && - (!args.Txn.Timestamp.Less(curIntent.Txn.Timestamp)) && (args.Txn.Sequence <= curIntent.Txn.Sequence) + + // Check whether the intent was pushed past its expected timestamp. + if reply.FoundIntent && args.Txn.Timestamp.Less(curIntent.Txn.Timestamp) { + // The intent matched but was pushed to a later timestamp. + curIntentPushed = true + + // If the transaction is SERIALIZABLE, consider a pushed intent as a + // missing intent. If the transaction is SNAPSHOT, don't. + if args.Txn.Isolation == enginepb.SERIALIZABLE { + reply.FoundIntent = false + } + + // If the request was querying an intent in its own transaction, update + // the response transaction. + if ownTxn { + clonedTxn := h.Txn.Clone() + reply.Txn = &clonedTxn + reply.Txn.Timestamp.Forward(curIntent.Txn.Timestamp) + } + } default: log.Fatalf(ctx, "more than 1 intent on single key: %v", intents) } @@ -86,6 +110,13 @@ func QueryIntent( case roachpb.QueryIntentRequest_DO_NOTHING: // Do nothing. case roachpb.QueryIntentRequest_RETURN_ERROR: + if ownTxn && curIntentPushed { + // If the transaction's own intent was pushed, go ahead and + // return a TransactionRetryError immediately with an updated + // transaction proto. This is an optimization that can help + // the txn use refresh spans more effectively. + return result.Result{}, roachpb.NewTransactionRetryError(roachpb.RETRY_SERIALIZABLE) + } return result.Result{}, roachpb.NewIntentMissingError(curIntent) case roachpb.QueryIntentRequest_PREVENT: // The intent will be prevented by bumping the timestamp cache for diff --git a/pkg/storage/engine/batch.go b/pkg/storage/engine/batch.go index b678a3bc9476..c32d66ef7c2c 100644 --- a/pkg/storage/engine/batch.go +++ b/pkg/storage/engine/batch.go @@ -407,7 +407,7 @@ func rocksDBBatchVarString(repr []byte) (s []byte, orepr []byte, err error) { // fmt.Printf("merge(%x,%x)", r.Key(), r.Value()) // } // } -// if err != nil { +// if err := r.Error(); err != nil { // return err // } type RocksDBBatchReader struct { diff --git a/pkg/storage/replica_test.go b/pkg/storage/replica_test.go index f80920bd9a81..1fec2793c720 100644 --- a/pkg/storage/replica_test.go +++ b/pkg/storage/replica_test.go @@ -5801,120 +5801,139 @@ func TestPushTxnSerializableRestart(t *testing.T) { func TestQueryIntentRequest(t *testing.T) { defer leaktest.AfterTest(t)() - for _, behavior := range []roachpb.QueryIntentRequest_IfMissingBehavior{ - roachpb.QueryIntentRequest_DO_NOTHING, - roachpb.QueryIntentRequest_RETURN_ERROR, - roachpb.QueryIntentRequest_PREVENT, - } { - t.Run(fmt.Sprintf("behavior=%s", behavior), func(t *testing.T) { - tc := testContext{} - stopper := stop.NewStopper() - defer stopper.Stop(context.TODO()) - tc.Start(t, stopper) + for _, iso := range []enginepb.IsolationType{enginepb.SNAPSHOT, enginepb.SERIALIZABLE} { + t.Run(iso.String(), func(t *testing.T) { + for _, behavior := range []roachpb.QueryIntentRequest_IfMissingBehavior{ + roachpb.QueryIntentRequest_DO_NOTHING, + roachpb.QueryIntentRequest_RETURN_ERROR, + roachpb.QueryIntentRequest_PREVENT, + } { + if iso == enginepb.SNAPSHOT && behavior == roachpb.QueryIntentRequest_PREVENT { + // Cannot prevent SNAPSHOT transaction with QueryIntent. + continue + } + t.Run(fmt.Sprintf("behavior=%s", behavior), func(t *testing.T) { - key1 := roachpb.Key("a") - key2 := roachpb.Key("b") - txn := newTransaction("test", key1, 1, enginepb.SERIALIZABLE, tc.Clock()) + tc := testContext{} + stopper := stop.NewStopper() + defer stopper.Stop(context.TODO()) + tc.Start(t, stopper) - pArgs := putArgs(key1, []byte("value1")) - assignSeqNumsForReqs(txn, &pArgs) - if _, pErr := tc.SendWrappedWith(roachpb.Header{Txn: txn}, &pArgs); pErr != nil { - t.Fatal(pErr) - } + key1 := roachpb.Key("a") + key2 := roachpb.Key("b") + txn := newTransaction("test", key1, 1, iso, tc.Clock()) + txn2 := newTransaction("test2", key2, 1, iso, tc.Clock()) - queryIntent := func( - key []byte, - txnMeta enginepb.TxnMeta, - baTxn *roachpb.Transaction, - expectIntent bool, - ) { - t.Helper() - qiArgs := queryIntentArgs(key, txnMeta, behavior) - qiRes, pErr := tc.SendWrappedWith(roachpb.Header{Txn: baTxn}, &qiArgs) - if behavior == roachpb.QueryIntentRequest_RETURN_ERROR && !expectIntent { - if _, ok := pErr.GetDetail().(*roachpb.IntentMissingError); !ok { - t.Fatalf("expected IntentMissingError, found %v", pErr) + pArgs := putArgs(key1, []byte("value1")) + assignSeqNumsForReqs(txn, &pArgs) + if _, pErr := tc.SendWrappedWith(roachpb.Header{Txn: txn}, &pArgs); pErr != nil { + t.Fatal(pErr) } - } else { + + queryIntent := func( + key []byte, + txnMeta enginepb.TxnMeta, + baTxn *roachpb.Transaction, + expectIntent bool, + ) { + t.Helper() + qiArgs := queryIntentArgs(key, txnMeta, behavior) + qiRes, pErr := tc.SendWrappedWith(roachpb.Header{Txn: baTxn}, &qiArgs) + if behavior == roachpb.QueryIntentRequest_RETURN_ERROR && !expectIntent { + ownIntent := baTxn != nil && baTxn.ID == txnMeta.ID + if ownIntent && txnMeta.Timestamp.Less(txn.Timestamp) { + if _, ok := pErr.GetDetail().(*roachpb.TransactionRetryError); !ok { + t.Fatalf("expected TransactionRetryError, found %v %v", txnMeta, pErr) + } + } else { + if _, ok := pErr.GetDetail().(*roachpb.IntentMissingError); !ok { + t.Fatalf("expected IntentMissingError, found %v", pErr) + } + } + } else { + if pErr != nil { + t.Fatal(pErr) + } + if e, a := expectIntent, qiRes.(*roachpb.QueryIntentResponse).FoundIntent; e != a { + t.Fatalf("expected FoundIntent=%t but FoundIntent=%t", e, a) + } + } + } + + for _, baTxn := range []*roachpb.Transaction{nil, txn, txn2} { + // Query the intent with the correct txn meta. Should see intent regardless + // of whether we're inside the txn or not. + queryIntent(key1, txn.TxnMeta, baTxn, true) + + // Query an intent on a different key for the same transaction. Should not + // see an intent. + queryIntent(key2, txn.TxnMeta, baTxn, false) + + // Query an intent on the same key for a different transaction. Should not + // see an intent. + diffIDMeta := txn.TxnMeta + diffIDMeta.ID = txn2.ID + queryIntent(key1, diffIDMeta, baTxn, false) + + // Query the intent with a larger epoch. Should not see an intent. + largerEpochMeta := txn.TxnMeta + largerEpochMeta.Epoch++ + queryIntent(key1, largerEpochMeta, baTxn, false) + + // Query the intent with a smaller epoch. Should not see an intent. + smallerEpochMeta := txn.TxnMeta + smallerEpochMeta.Epoch-- + queryIntent(key1, smallerEpochMeta, baTxn, false) + + // Query the intent with a larger timestamp. Should see an intent. + // See the comment on QueryIntentRequest.Txn for an explanation of why + // the request behaves like this. + largerTSMeta := txn.TxnMeta + largerTSMeta.Timestamp = largerTSMeta.Timestamp.Next() + queryIntent(key1, largerTSMeta, baTxn, true) + + // Query the intent with a smaller timestamp. Should not see an intent + // if transaction is serializable. Should see an intent if the transaction + // is SNAPSHOT. + smallerTSMeta := txn.TxnMeta + smallerTSMeta.Timestamp = smallerTSMeta.Timestamp.Prev() + queryIntent(key1, smallerTSMeta, baTxn, iso == enginepb.SNAPSHOT) + + // Query the intent with a larger sequence number. Should not see an intent. + largerSeqMeta := txn.TxnMeta + largerSeqMeta.Sequence++ + queryIntent(key1, largerSeqMeta, baTxn, false) + + // Query the intent with a smaller sequence number. Should see an intent. + // See the comment on QueryIntentRequest.Txn for an explanation of why + // the request behaves like this. + smallerSeqMeta := txn.TxnMeta + smallerSeqMeta.Sequence-- + queryIntent(key1, smallerSeqMeta, baTxn, true) + } + + // Perform a write at key2. Depending on the behavior of the queryIntent + // that queried that key, this write should have different results. + pArgs2 := putArgs(key2, []byte("value2")) + assignSeqNumsForReqs(txn, &pArgs2) + ba := roachpb.BatchRequest{} + ba.Header = roachpb.Header{Txn: txn} + ba.Add(&pArgs2) + br, pErr := tc.Sender().Send(context.Background(), ba) if pErr != nil { t.Fatal(pErr) } - if e, a := expectIntent, qiRes.(*roachpb.QueryIntentResponse).FoundIntent; e != a { - t.Fatalf("expected FoundIntent=%T but FoundIntent=%T", e, a) + tsBumped := br.Txn.Timestamp != br.Txn.OrigTimestamp + if behavior == roachpb.QueryIntentRequest_PREVENT { + if !tsBumped { + t.Fatalf("transaction timestamp not bumped: %v", br.Txn) + } + } else { + if tsBumped { + t.Fatalf("unexpected transaction timestamp bumped: %v", br.Txn) + } } - } - } - - for _, baTxn := range []*roachpb.Transaction{nil, txn} { - // Query the intent with the correct txn meta. Should see intent regardless - // of whether we're inside the txn or not. - queryIntent(key1, txn.TxnMeta, baTxn, true) - - // Query an intent on a different key for the same transaction. Should not - // see an intent. - queryIntent(key2, txn.TxnMeta, baTxn, false) - - // Query an intent on the same key for a different transaction. Should not - // see an intent. - diffIDMeta := txn.TxnMeta - diffIDMeta.ID = uuid.MakeV4() - queryIntent(key1, diffIDMeta, baTxn, false) - - // Query the intent with a larger epoch. Should not see an intent. - largerEpochMeta := txn.TxnMeta - largerEpochMeta.Epoch++ - queryIntent(key1, largerEpochMeta, baTxn, false) - - // Query the intent with a smaller epoch. Should not see an intent. - smallerEpochMeta := txn.TxnMeta - smallerEpochMeta.Epoch-- - queryIntent(key1, smallerEpochMeta, baTxn, false) - - // Query the intent with a larger timestamp. Should see an intent. - // See the comment on QueryIntentRequest.Txn for an explanation of why - // the request behaves like this. - largerTSMeta := txn.TxnMeta - largerTSMeta.Timestamp = largerTSMeta.Timestamp.Next() - queryIntent(key1, largerTSMeta, baTxn, true) - - // Query the intent with a smaller timestamp. Should not see an intent. - smallerTSMeta := txn.TxnMeta - smallerTSMeta.Timestamp = smallerTSMeta.Timestamp.Prev() - queryIntent(key1, smallerTSMeta, baTxn, false) - - // Query the intent with a larger sequence number. Should not see an intent. - largerSeqMeta := txn.TxnMeta - largerSeqMeta.Sequence++ - queryIntent(key1, largerSeqMeta, baTxn, false) - - // Query the intent with a smaller sequence number. Should see an intent. - // See the comment on QueryIntentRequest.Txn for an explanation of why - // the request behaves like this. - smallerSeqMeta := txn.TxnMeta - smallerSeqMeta.Sequence-- - queryIntent(key1, smallerSeqMeta, baTxn, true) - } - - // Perform a write at key2. Depending on the behavior of the queryIntent - // that queried that key, this write should have different results. - pArgs2 := putArgs(key2, []byte("value2")) - assignSeqNumsForReqs(txn, &pArgs2) - ba := roachpb.BatchRequest{} - ba.Header = roachpb.Header{Txn: txn} - ba.Add(&pArgs2) - br, pErr := tc.Sender().Send(context.Background(), ba) - if pErr != nil { - t.Fatal(pErr) - } - tsBumped := br.Txn.Timestamp != br.Txn.OrigTimestamp - if behavior == roachpb.QueryIntentRequest_PREVENT { - if !tsBumped { - t.Fatalf("transaction timestamp not bumped: %v", br.Txn) - } - } else { - if tsBumped { - t.Fatalf("unexpected transaction timestamp bumped: %v", br.Txn) - } + }) } }) } diff --git a/pkg/ui/README.md b/pkg/ui/README.md index a33cb71c1f56..23c978ebed8f 100644 --- a/pkg/ui/README.md +++ b/pkg/ui/README.md @@ -16,7 +16,7 @@ you'll be able to access the UI at . Our UI is compiled using a collection of tools that depends on [Node.js](https://nodejs.org/) and are managed with [Yarn](https://yarnpkg.com), a package manager that offers more deterministic -package installation than NPM. NodeJS 6.x and Yarn 0.22.0 are known to work. +package installation than NPM. NodeJS 6.x and Yarn 1.7.0 are known to work. [Chrome](https://www.google.com/chrome/), Google's internet browser. Unit tests are run using Chrome's "Headless" mode. diff --git a/pkg/ui/package.json b/pkg/ui/package.json index 4489dac8a625..fdc33f8aa3a9 100644 --- a/pkg/ui/package.json +++ b/pkg/ui/package.json @@ -111,6 +111,6 @@ }, "engines": { "node": ">=6", - "yarn": ">=1.0.0" + "yarn": ">=1.7.0" } } diff --git a/pkg/util/hlc/hlc_test.go b/pkg/util/hlc/hlc_test.go index 9dacd2f21024..1409ec29914b 100644 --- a/pkg/util/hlc/hlc_test.go +++ b/pkg/util/hlc/hlc_test.go @@ -17,7 +17,6 @@ package hlc import ( "context" "fmt" - "os" "regexp" "testing" "time" @@ -115,8 +114,8 @@ func isErrSimilar(expected *regexp.Regexp, actual error) bool { func TestHLCPhysicalClockJump(t *testing.T) { var fatal bool - defer log.SetExitFunc(os.Exit) - log.SetExitFunc(func(r int) { + defer log.ResetExitFunc() + log.SetExitFunc(true /* hideStack */, func(r int) { if r != 0 { fatal = true } @@ -372,8 +371,8 @@ func TestHLCMonotonicityCheck(t *testing.T) { func TestHLCEnforceWallTimeWithinBoundsInNow(t *testing.T) { var fatal bool - defer log.SetExitFunc(os.Exit) - log.SetExitFunc(func(r int) { + defer log.ResetExitFunc() + log.SetExitFunc(true /* hideStack */, func(r int) { defer log.Flush() if r != 0 { fatal = true @@ -421,8 +420,8 @@ func TestHLCEnforceWallTimeWithinBoundsInNow(t *testing.T) { func TestHLCEnforceWallTimeWithinBoundsInUpdate(t *testing.T) { var fatal bool - defer log.SetExitFunc(os.Exit) - log.SetExitFunc(func(r int) { + defer log.ResetExitFunc() + log.SetExitFunc(true /* hideStack */, func(r int) { defer log.Flush() if r != 0 { fatal = true diff --git a/pkg/util/log/clog.go b/pkg/util/log/clog.go index 130804ecfdfc..ade7ae76098c 100644 --- a/pkg/util/log/clog.go +++ b/pkg/util/log/clog.go @@ -584,7 +584,6 @@ func init() { logging.prefix = program logging.setVState(0, nil, false) - logging.exitFunc = os.Exit logging.gcNotify = make(chan struct{}, 1) logging.fatalCh = make(chan struct{}) @@ -689,11 +688,14 @@ type loggingT struct { disableDaemons bool // These flags are modified only under lock, although verbosity may be fetched // safely using atomic.LoadInt32. - vmodule moduleSpec // The state of the --vmodule flag. - verbosity level // V logging level, the value of the --verbosity flag/ - exitFunc func(int) // func that will be called on fatal errors - gcNotify chan struct{} // notify GC daemon that a new log file was created - fatalCh chan struct{} // closed on fatal error + vmodule moduleSpec // The state of the --vmodule flag. + verbosity level // V logging level, the value of the --verbosity flag/ + exitOverride struct { + f func(int) // overrides os.Exit when non-nil; testing only + hideStack bool // hides stack trace; only in effect when f is not nil + } + gcNotify chan struct{} // notify GC daemon that a new log file was created + fatalCh chan struct{} // closed on fatal error interceptor atomic.Value // InterceptorFn @@ -848,8 +850,15 @@ func (l *loggingT) outputLogEntry(s Severity, file string, line int, msg string) // // https://github.com/cockroachdb/cockroach/issues/23119 fatalTrigger = make(chan struct{}) - exitFunc := l.exitFunc + exitFunc := os.Exit + if l.exitOverride.f != nil { + if l.exitOverride.hideStack { + stacks = []byte("stack trace omitted via SetExitFunc)\n") + } + exitFunc = l.exitOverride.f + } exitCalled := make(chan struct{}) + defer func() { <-exitCalled }() @@ -998,7 +1007,11 @@ func (l *loggingT) exitLocked(err error) { return } l.flushAndSync(true /*doSync*/) - l.exitFunc(2) + if l.exitOverride.f != nil { + l.exitOverride.f(2) + } else { + os.Exit(2) + } } // syncBuffer joins a bufio.Writer to its underlying file, providing access to the diff --git a/pkg/util/log/clog_test.go b/pkg/util/log/clog_test.go index 68ac7166621d..788f9b528a6a 100644 --- a/pkg/util/log/clog_test.go +++ b/pkg/util/log/clog_test.go @@ -92,10 +92,9 @@ func contains(str string, t *testing.T) bool { return strings.Contains(c, str) } -// setFlags configures the logging flags and exitFunc how the test expects -// them. +// setFlags resets the logging flags and exit function to what tests expect. func setFlags() { - SetExitFunc(os.Exit) + ResetExitFunc() logging.mu.Lock() defer logging.mu.Unlock() logging.noStderrRedirect = false @@ -699,7 +698,7 @@ func TestFatalStacktraceStderr(t *testing.T) { setFlags() logging.stderrThreshold = Severity_NONE - SetExitFunc(func(int) {}) + SetExitFunc(false /* hideStack */, func(int) {}) defer setFlags() defer logging.swap(logging.newBuffers()) @@ -790,11 +789,11 @@ func TestExitOnFullDisk(t *testing.T) { var exited sync.WaitGroup exited.Add(1) - l := &loggingT{ - exitFunc: func(int) { - exited.Done() - }, + l := &loggingT{} + l.exitOverride.f = func(int) { + exited.Done() } + l.file = &syncBuffer{ logger: l, Writer: bufio.NewWriterSize(&outOfSpaceWriter{}, 1), diff --git a/pkg/util/log/log.go b/pkg/util/log/log.go index b948b67b9520..209e883e9650 100644 --- a/pkg/util/log/log.go +++ b/pkg/util/log/log.go @@ -40,11 +40,25 @@ func FatalOnPanic() { } // SetExitFunc allows setting a function that will be called to exit the -// process when a Fatal message is generated. -func SetExitFunc(f func(int)) { +// process when a Fatal message is generated. The supplied bool, if true, +// suppresses the stack trace, which is useful for test callers wishing +// to keep the logs reasonably clean. +// +// Call with a nil function to undo. +func SetExitFunc(hideStack bool, f func(int)) { + logging.mu.Lock() + defer logging.mu.Unlock() + logging.exitOverride.f = f + logging.exitOverride.hideStack = hideStack +} + +// ResetExitFunc undoes any prior call to SetExitFunc. +func ResetExitFunc() { logging.mu.Lock() defer logging.mu.Unlock() - logging.exitFunc = f + + logging.exitOverride.f = nil + logging.exitOverride.hideStack = false } // logDepth uses the PrintWith to format the output string and diff --git a/pkg/util/log/secondary_log.go b/pkg/util/log/secondary_log.go index db027f81b1c5..a8b92c298cb7 100644 --- a/pkg/util/log/secondary_log.go +++ b/pkg/util/log/secondary_log.go @@ -17,7 +17,6 @@ package log import ( "context" "fmt" - "os" "sync/atomic" "github.com/cockroachdb/cockroach/pkg/util/caller" @@ -67,7 +66,6 @@ func NewSecondaryLogger( syncWrites: forceSyncWrites || logging.syncWrites, gcNotify: make(chan struct{}, 1), disableDaemons: logging.disableDaemons, - exitFunc: os.Exit, }, forceSyncWrites: forceSyncWrites, }