Fixing cancel race condition (#92)

Because we were spanning a new coroutine to cancel the request, the
connection was closing and in parallel the sentinel was trying to cancel

In http_client the coroutines were racing to closeResponse

This PR fixes the issue and adds a new action to run race condition
tests.

Signed-off-by: Andre Furlan <[email protected]>

Author: andrefurlan-db

.github/workflows/go.yml

@@ -65,5 +65,8 @@ jobs:
         env:
           CGO_ENABLED: 0
 
+      - name: Test-Race
+        run: make test-race
+
       - name: Build
         run: make linux

Makefile

@@ -57,6 +57,12 @@ test: bin/gotestsum  ## Run the go unit tests.
 	@echo "INFO: Running all go unit tests."
 	CGO_ENABLED=0 ./bin/gotestsum --format pkgname-and-test-fails --junitfile $(TEST_RESULTS_DIR)/unit-tests.xml ./...
 
+.PHONY: test-race
+test-race:
+	@echo "INFO: Running all go unit tests checking for race conditions."
+	go test -race
+
+
 .PHONY: coverage
 coverage: bin/gotestsum  ## Report the unit test code coverage.
 	@echo "INFO: Generating unit test coverage report."

connection_test.go

@@ -544,7 +544,6 @@ func TestConn_pollOperation(t *testing.T) {
 				Secret: []byte("b"),
 			},
 		})
-		time.Sleep(50 * time.Millisecond)
 		assert.Error(t, err)
 		assert.Equal(t, 0, getOperationStatusCount)
 		assert.Equal(t, 1, cancelOperationCount)
@@ -590,7 +589,7 @@ func TestConn_pollOperation(t *testing.T) {
 				Secret: []byte("b"),
 			},
 		})
-		time.Sleep(50 * time.Millisecond)
+
 		assert.Error(t, err)
 		assert.GreaterOrEqual(t, getOperationStatusCount, 1)
 		assert.Equal(t, 1, cancelOperationCount)
@@ -1183,7 +1182,6 @@ func TestConn_ExecContext(t *testing.T) {
 		ctx, cancel = context.WithCancel(ctx)
 		defer cancel()
 		res, err := testConn.ExecContext(ctx, "insert 10", []driver.NamedValue{})
-		time.Sleep(10 * time.Millisecond)
 		assert.Error(t, err)
 		assert.Nil(t, res)
 		assert.Equal(t, 1, executeStatementCount)

driver_e2e_test.go

@@ -291,7 +291,6 @@ func TestContextTimeoutExample(t *testing.T) {
 	assert.True(t, ok)
 	assert.Equal(t, 1, state.executeStatementCalls)
 	assert.GreaterOrEqual(t, state.getOperationStatusCalls, 1)
-	time.Sleep(time.Second)
 	assert.Equal(t, 1, state.cancelOperationCalls)
 
 }

internal/sentinel/sentinel.go

@@ -83,15 +83,6 @@ func (s Sentinel) Watch(ctx context.Context, interval, timeout time.Duration) (W
 			resCh <- ret
 		}
 	}
-	canceler := func(ctx context.Context, reason string) {
-		_, err := s.OnCancelFn()
-		if err != nil {
-			log.Err(err).Msgf("cancel failed after %s", reason)
-			return
-		}
-		log.Debug().Msgf("cancel success")
-	}
-
 	for {
 		select {
 		case <-intervalTimer.C:
@@ -117,7 +108,12 @@ func (s Sentinel) Watch(ctx context.Context, interval, timeout time.Duration) (W
 			_ = intervalTimer.Stop()
 			if s.OnCancelFn != nil && !s.onCancelFnCalled {
 				s.onCancelFnCalled = true
-				go canceler(ctx, ctx.Err().Error())
+				_, err := s.OnCancelFn()
+				if err != nil {
+					log.Err(err).Msg("cancel failed")
+					return WatchCanceled, nil, errors.Wrap(err, ctx.Err().Error())
+				}
+				log.Debug().Msgf("cancel success")
 			}
 			return WatchCanceled, nil, errors.Wrap(ctx.Err(), "sentinel context done")
 		case <-timeoutTimerCh:
@@ -126,8 +122,14 @@ func (s Sentinel) Watch(ctx context.Context, interval, timeout time.Duration) (W
 			_ = intervalTimer.Stop()
 			if s.OnCancelFn != nil && !s.onCancelFnCalled {
 				s.onCancelFnCalled = true
-				go canceler(ctx, err.Error())
+				_, err := s.OnCancelFn()
+				if err != nil {
+					log.Err(err).Msg("cancel failed")
+					return WatchCanceled, nil, errors.Wrap(err, ctx.Err().Error())
+				}
+				log.Debug().Msgf("cancel success")
 			}
+
 			return WatchTimeout, nil, err
 		}
 	}

internal/sentinel/sentinel_test.go

@@ -98,7 +98,6 @@ func TestWatch(t *testing.T) {
 			},
 		}
 		status, res, err := s.Watch(context.Background(), 0, 100*time.Millisecond)
-		time.Sleep(10 * time.Millisecond)
 		assert.Equal(t, WatchTimeout, status)
 		assert.Equal(t, 1, cancelFnCalls)
 		assert.Nil(t, res)
@@ -167,7 +166,6 @@ func TestWatch(t *testing.T) {
 		}
 		status, res, err := s.Watch(ctx, 0, 15*time.Second)
 		assert.Equal(t, WatchCanceled, status)
-		time.Sleep(10 * time.Millisecond)
 		assert.Equal(t, cancelFnCalls, 1)
 		assert.Nil(t, res)
 		assert.Error(t, err)
@@ -194,7 +192,6 @@ func TestWatch(t *testing.T) {
 		}
 		status, res, err := s.Watch(ctx, 0, 200*time.Millisecond)
 		assert.Equal(t, WatchCanceled, status)
-		time.Sleep(10 * time.Millisecond)
 		assert.Equal(t, cancelFnCalls, 1)
 		assert.Nil(t, res)
 		assert.Error(t, err)
@@ -273,7 +270,6 @@ func TestWatch(t *testing.T) {
 			},
 		}
 		status, res, err := s.Watch(context.Background(), 0, 0)
-		time.Sleep(10 * time.Millisecond)
 		assert.Equal(t, WatchErr, status)
 		assert.Equal(t, 1, statusFnCalls)
 		assert.Nil(t, res)