From 729b65160b023da56441093db91b07507b0e5956 Mon Sep 17 00:00:00 2001 From: Andrew Mason Date: Mon, 24 Jul 2023 13:39:12 -0400 Subject: [PATCH] Fix potential panics due to "Fail in goroutine after test completed" The root issue here is that some of the functions we call before the `cancel(); wg.Wait()` section include calls to assertions from the `require` subpackage, which, unlike their `assert` counterparts, end up calling `t.FailNow()` which aborts the test run via `runtime.Goexit()` If this happens while another goroutine is still running, the test "completes", and if that other goroutine has any additional assertions that fail, this panic occurs. Luckily, `runtime.GoExit` still executes the deferred function calls in the exiting goroutine, so as long as we defer the context cancelation and wait, we will ensure that that other goroutine exits before the main goroutine of the test case. Since these tests sometimes need to run code both before and after the "cancel and wait for goroutine" section, we need to doubly-nest a function definition to ensure we have a defer and it executes when we want it to. Signed-off-by: Andrew Mason --- .../onlineddl/revert/onlineddl_revert_test.go | 88 ++++++++++++++----- 1 file changed, 66 insertions(+), 22 deletions(-) diff --git a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go index dd0b6d84a53..f168efa75e4 100644 --- a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go +++ b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go @@ -764,11 +764,21 @@ func testRevert(t *testing.T) { defer wg.Done() runMultipleConnections(ctx, t) }() - uuid := testOnlineDDLStatementForTable(t, fmt.Sprintf(alterHintStatement, hint), "online", "vtgate", hint) - uuids = append(uuids, uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) - cancel() // will cause runMultipleConnections() to terminate - wg.Wait() + + func() { + // Ensures runMultipleConnections completes before the overall + // test does, even in the face of calls to t.FailNow() in the + // main goroutine, which still executes deferred functions + defer func() { + cancel() // will cause runMultipleConnections() to terminate + wg.Wait() + }() + + uuid := testOnlineDDLStatementForTable(t, fmt.Sprintf(alterHintStatement, hint), "online", "vtgate", hint) + uuids = append(uuids, uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + }() + testSelectTableMetrics(t) }) } @@ -783,11 +793,20 @@ func testRevert(t *testing.T) { defer wg.Done() runMultipleConnections(ctx, t) }() - uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) - uuids = append(uuids, uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) - cancel() // will cause runMultipleConnections() to terminate - wg.Wait() + + func() { + // Ensures runMultipleConnections completes before the overall + // test does, even in the face of calls to t.FailNow() in the + // main goroutine, which still executes deferred functions + defer func() { + cancel() // will cause runMultipleConnections() to terminate + wg.Wait() + }() + + uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) + uuids = append(uuids, uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + }() checkMigratedTable(t, tableName, alterHints[0]) testSelectTableMetrics(t) }) @@ -802,11 +821,20 @@ func testRevert(t *testing.T) { defer wg.Done() runMultipleConnections(ctx, t) }() - uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) - uuids = append(uuids, uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) - cancel() // will cause runMultipleConnections() to terminate - wg.Wait() + + func() { + // Ensures runMultipleConnections completes before the overall + // test does, even in the face of calls to t.FailNow() in the + // main goroutine, which still executes deferred functions + defer func() { + cancel() // will cause runMultipleConnections() to terminate + wg.Wait() + }() + + uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) + uuids = append(uuids, uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + }() checkMigratedTable(t, tableName, alterHints[1]) testSelectTableMetrics(t) }) @@ -821,11 +849,20 @@ func testRevert(t *testing.T) { defer wg.Done() runMultipleConnections(ctx, t) }() - uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) - uuids = append(uuids, uuid) - onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) - cancel() // will cause runMultipleConnections() to terminate - wg.Wait() + + func() { + // Ensures runMultipleConnections completes before the overall + // test does, even in the face of calls to t.FailNow() in the + // main goroutine, which still executes deferred functions + defer func() { + cancel() // will cause runMultipleConnections() to terminate + wg.Wait() + }() + + uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy) + uuids = append(uuids, uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + }() checkMigratedTable(t, tableName, alterHints[0]) testSelectTableMetrics(t) }) @@ -839,6 +876,15 @@ func testRevert(t *testing.T) { defer wg.Done() runMultipleConnections(ctx, t) }() + + // Ensures runMultipleConnections completes before the overall + // test does, even in the face of calls to t.FailNow() in the + // main goroutine, which still executes deferred functions + defer func() { + cancel() // will cause runMultipleConnections() to terminate + wg.Wait() + }() + uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy+" --postpone-completion") uuids = append(uuids, uuid) // Should be still running! @@ -849,8 +895,6 @@ func testRevert(t *testing.T) { status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, uuid, 60*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed) fmt.Printf("# Migration status (for debug purposes): <%s>\n", status) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) - cancel() // will cause runMultipleConnections() to terminate - wg.Wait() } t.Run("postponed revert", func(t *testing.T) { testPostponedRevert(t, schema.OnlineDDLStatusRunning)