Fix closing the body for HTTP requests (#11842) (#11853)

.golangci.yml

@@ -18,20 +18,18 @@ linters:
  disable-all: true
  enable:
  # Defaults
- - deadcode
  - errcheck
  - govet
  - ineffassign
- - structcheck
  - typecheck
- - varcheck
  - staticcheck
  - gosimple
 
  # Extras
  - gofmt
  - goimports
  - exportloopref
+ - bodyclose
 
  # revive is a replacement for golint, but we do not run it in CI for now.
  # This is only enabled as a post-commit hook

go/test/endtoend/clustertest/main_test.go

@@ -107,9 +107,10 @@ func testURL(t *testing.T, url string, testCaseName string) {
 
 // getStatusForUrl returns the status code for the URL
 func getStatusForURL(url string) int {
- resp, _ := http.Get(url)
- if resp != nil {
- return resp.StatusCode
+ resp, err := http.Get(url)
+ if err != nil {
+ return 0
  }
- return 0
+ defer resp.Body.Close()
+ return resp.StatusCode
 }

go/test/endtoend/clustertest/vtctld_test.go

@@ -62,13 +62,15 @@ func TestVtctldProcess(t *testing.T) {
 
 func testTopoDataAPI(t *testing.T, url string) {
  resp, err := http.Get(url)
- require.Nil(t, err)
+ require.NoError(t, err)
+ defer resp.Body.Close()
  assert.Equal(t, resp.StatusCode, 200)
 
  resultMap := make(map[string]any)
- respByte, _ := io.ReadAll(resp.Body)
+ respByte, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
  err = json.Unmarshal(respByte, &resultMap)
- require.Nil(t, err)
+ require.NoError(t, err)
 
  errorValue := reflect.ValueOf(resultMap["Error"])
  assert.Empty(t, errorValue.String())
@@ -83,7 +85,7 @@ func testTopoDataAPI(t *testing.T, url string) {
 func testListAllTablets(t *testing.T) {
  // first w/o any filters, aside from cell
  result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ListAllTablets", clusterInstance.Cell)
- require.Nil(t, err)
+ require.NoError(t, err)
 
  tablets := getAllTablets()
 
@@ -104,7 +106,7 @@ func testListAllTablets(t *testing.T) {
  "ListAllTablets", "--", "--keyspace", clusterInstance.Keyspaces[0].Name,
  "--tablet_type", "primary",
  clusterInstance.Cell)
- require.Nil(t, err)
+ require.NoError(t, err)
 
  // We should only return a single primary tablet per shard in the first keyspace
  tabletsFromCMD = strings.Split(result, "\n")
@@ -115,9 +117,10 @@ func testListAllTablets(t *testing.T) {
 
 func testTabletStatus(t *testing.T) {
  resp, err := http.Get(fmt.Sprintf("http://%s:%d", clusterInstance.Hostname, clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].HTTPPort))
- require.Nil(t, err)
+ require.NoError(t, err)
+ defer resp.Body.Close()
  respByte, err := io.ReadAll(resp.Body)
- require.Nil(t, err)
+ require.NoError(t, err)
  result := string(respByte)
  log.Infof("Tablet status response: %v", result)
  assert.True(t, strings.Contains(result, `Alias: <a href="http://localhost:`))
@@ -126,13 +129,13 @@ func testTabletStatus(t *testing.T) {
 
 func testExecuteAsDba(t *testing.T) {
  result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ExecuteFetchAsDba", clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].Alias, `SELECT 1 AS a`)
- require.Nil(t, err)
+ require.NoError(t, err)
  assert.Equal(t, result, oneTableOutput)
 }
 
 func testExecuteAsApp(t *testing.T) {
  result, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("ExecuteFetchAsApp", clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].Alias, `SELECT 1 AS a`)
- require.Nil(t, err)
+ require.NoError(t, err)
  assert.Equal(t, result, oneTableOutput)
 }
 

go/test/endtoend/clustertest/vtgate_test.go

@@ -40,7 +40,7 @@ func TestVtgateProcess(t *testing.T) {
  verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
  ctx := context.Background()
  conn, err := mysql.Connect(ctx, &vtParams)
- require.Nil(t, err)
+ require.NoError(t, err)
  defer conn.Close()
 
  utils.Exec(t, conn, "insert into customer(id, email) values(1,'email1')")
@@ -52,41 +52,42 @@ func TestVtgateProcess(t *testing.T) {
 }
 
 func verifyVtgateVariables(t *testing.T, url string) {
- resp, _ := http.Get(url)
- if resp != nil && resp.StatusCode == 200 {
- resultMap := make(map[string]any)
- respByte, _ := io.ReadAll(resp.Body)
- err := json.Unmarshal(respByte, &resultMap)
- require.Nil(t, err)
- if resultMap["VtgateVSchemaCounts"] == nil {
- t.Error("Vschema count should be present in variables")
- }
- vschemaCountMap := getMapFromJSON(resultMap, "VtgateVSchemaCounts")
- if _, present := vschemaCountMap["Reload"]; !present {
- t.Error("Reload count should be present in vschemacount")
- } else if object := reflect.ValueOf(vschemaCountMap["Reload"]); object.NumField() <= 0 {
- t.Error("Reload count should be greater than 0")
- }
- if _, present := vschemaCountMap["WatchError"]; present {
- t.Error("There should not be any WatchError in VschemaCount")
- }
- if _, present := vschemaCountMap["Parsing"]; present {
- t.Error("There should not be any Parsing in VschemaCount")
- }
+ resp, err := http.Get(url)
+ require.NoError(t, err)
+ defer resp.Body.Close()
 
- if resultMap["HealthcheckConnections"] == nil {
- t.Error("HealthcheckConnections count should be present in variables")
- }
+ require.Equal(t, 200, resp.StatusCode)
+ resultMap := make(map[string]any)
+ respByte, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ err = json.Unmarshal(respByte, &resultMap)
+ require.NoError(t, err)
+ if resultMap["VtgateVSchemaCounts"] == nil {
+ t.Error("Vschema count should be present in variables")
+ }
+ vschemaCountMap := getMapFromJSON(resultMap, "VtgateVSchemaCounts")
+ if _, present := vschemaCountMap["Reload"]; !present {
+ t.Error("Reload count should be present in vschemacount")
+ } else if object := reflect.ValueOf(vschemaCountMap["Reload"]); object.NumField() <= 0 {
+ t.Error("Reload count should be greater than 0")
+ }
+ if _, present := vschemaCountMap["WatchError"]; present {
+ t.Error("There should not be any WatchError in VschemaCount")
+ }
+ if _, present := vschemaCountMap["Parsing"]; present {
+ t.Error("There should not be any Parsing in VschemaCount")
+ }
 
- healthCheckConnection := getMapFromJSON(resultMap, "HealthcheckConnections")
- if len(healthCheckConnection) <= 0 {
- t.Error("Atleast one healthy tablet needs to be present")
- }
- if !isPrimaryTabletPresent(healthCheckConnection) {
- t.Error("Atleast one PRIMARY tablet needs to be present")
- }
- } else {
- t.Error("Vtgate api url response not found")
+ if resultMap["HealthcheckConnections"] == nil {
+ t.Error("HealthcheckConnections count should be present in variables")
+ }
+
+ healthCheckConnection := getMapFromJSON(resultMap, "HealthcheckConnections")
+ if len(healthCheckConnection) <= 0 {
+ t.Error("Atleast one healthy tablet needs to be present")
+ }
+ if !isPrimaryTabletPresent(healthCheckConnection) {
+ t.Error("Atleast one PRIMARY tablet needs to be present")
  }
 }
 

go/test/endtoend/clustertest/vttablet_test.go

@@ -33,13 +33,15 @@ func TestVttabletProcess(t *testing.T) {
  defer cluster.PanicHandler(t)
  firstTabletPort := clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].HTTPPort
  testURL(t, fmt.Sprintf("http://localhost:%d/debug/vars/", firstTabletPort), "tablet debug var url")
- resp, _ := http.Get(fmt.Sprintf("http://localhost:%d/debug/vars", firstTabletPort))
+ resp, err := http.Get(fmt.Sprintf("http://localhost:%d/debug/vars", firstTabletPort))
+ require.NoError(t, err)
+ defer resp.Body.Close()
+
  resultMap := make(map[string]any)
- respByte, _ := io.ReadAll(resp.Body)
- err := json.Unmarshal(respByte, &resultMap)
- if err != nil {
- panic(err)
- }
+ respByte, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ err = json.Unmarshal(respByte, &resultMap)
+ require.NoError(t, err)
  if got, want := resultMap["TabletKeyspace"], "commerce"; got != want {
  t.Errorf("select:\n%v want\n%v for %s", got, want, "Keyspace of tablet should match")
  }
@@ -50,5 +52,5 @@ func TestDeleteTablet(t *testing.T) {
  primary := clusterInstance.Keyspaces[0].Shards[0].PrimaryTablet()
  require.NotNil(t, primary)
  _, err := clusterInstance.VtctlclientProcess.ExecuteCommandWithOutput("DeleteTablet", "--", "--allow_primary", primary.Alias)
- require.Nil(t, err, "Error: %v", err)
+ require.NoError(t, err)
 }

go/test/endtoend/messaging/message_test.go

@@ -648,8 +648,13 @@ func parseDebugVars(t *testing.T, output interface{}, vttablet *cluster.Vttablet
  if err != nil {
  t.Fatalf("failed to fetch %q: %v", debugVarURL, err)
  }
+ defer resp.Body.Close()
+
+ respByte, err := io.ReadAll(resp.Body)
+ if err != nil {
+ t.Fatalf("failed to read body %q: %v", debugVarURL, err)
+ }
 
- respByte, _ := io.ReadAll(resp.Body)
  if resp.StatusCode != 200 {
  t.Fatalf("status code %d while fetching %q:\n%s", resp.StatusCode, debugVarURL, respByte)
  }

go/test/endtoend/onlineddl/vrepl/onlineddl_vrepl_test.go

@@ -20,7 +20,6 @@ import (
  "flag"
  "fmt"
  "io"
- "net/http"
  "os"
  "path"
  "strings"
@@ -221,24 +220,25 @@ func TestMain(m *testing.M) {
 }
 
 // direct per-tablet throttler API instruction
-func throttleResponse(tablet *cluster.Vttablet, path string) (resp *http.Response, respBody string, err error) {
+func throttleResponse(tablet *cluster.Vttablet, path string) (respBody string, err error) {
  apiURL := fmt.Sprintf("http://%s:%d/%s", tablet.VttabletProcess.TabletHostname, tablet.HTTPPort, path)
- resp, err = httpClient.Get(apiURL)
+ resp, err := httpClient.Get(apiURL)
  if err != nil {
- return resp, respBody, err
+ return "", err
  }
+ defer resp.Body.Close()
  b, err := io.ReadAll(resp.Body)
  respBody = string(b)
- return resp, respBody, err
+ return respBody, err
 }
 
 // direct per-tablet throttler API instruction
-func throttleApp(tablet *cluster.Vttablet, app string) (*http.Response, string, error) {
+func throttleApp(tablet *cluster.Vttablet, app string) (string, error) {
  return throttleResponse(tablet, fmt.Sprintf("throttler/throttle-app?app=%s&duration=1h", app))
 }
 
 // direct per-tablet throttler API instruction
-func unthrottleApp(tablet *cluster.Vttablet, app string) (*http.Response, string, error) {
+func unthrottleApp(tablet *cluster.Vttablet, app string) (string, error) {
  return throttleResponse(tablet, fmt.Sprintf("throttler/unthrottle-app?app=%s", app))
 }
 
@@ -398,7 +398,7 @@ func TestSchemaChange(t *testing.T) {
  // vstreamer source; but it's OK to be on the safe side and throttle on all tablets. Doesn't
  // change the essence of this test.
  for _, tablet := range shard.Vttablets {
- _, body, err := throttleApp(tablet, vstreamerThrottlerAppName)
+ body, err := throttleApp(tablet, vstreamerThrottlerAppName)
  defer unthrottleApp(tablet, vstreamerThrottlerAppName)
 
  assert.NoError(t, err)
@@ -498,12 +498,12 @@ func TestSchemaChange(t *testing.T) {
  case 0:
  // this is the shard where we run PRS
  // Use per-tablet throttling API
- _, body, err = throttleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
+ body, err = throttleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
  defer unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
  case 1:
  // no PRS on this shard
  // Use per-tablet throttling API
- _, body, err = throttleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
+ body, err = throttleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
  defer unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
  }
  assert.NoError(t, err)
@@ -555,11 +555,11 @@ func TestSchemaChange(t *testing.T) {
  case 0:
  // this is the shard where we run PRS
  // Use per-tablet throttling API
- _, body, err = unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
+ body, err = unthrottleApp(shards[i].Vttablets[currentPrimaryTabletIndex], onlineDDLThrottlerAppName)
  case 1:
  // no PRS on this shard
  // Use per-tablet throttling API
- _, body, err = unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
+ body, err = unthrottleApp(shards[i].Vttablets[0], onlineDDLThrottlerAppName)
  }
  assert.NoError(t, err)
  assert.Contains(t, body, onlineDDLThrottlerAppName)
@@ -684,7 +684,7 @@ func TestSchemaChange(t *testing.T) {
  // shard 0 will run normally, shard 1 will be throttled
  defer unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
  t.Run("throttle shard 1", func(t *testing.T) {
- _, body, err := throttleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
+ body, err := throttleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
  assert.NoError(t, err)
  assert.Contains(t, body, onlineDDLThrottlerAppName)
  })
@@ -708,7 +708,7 @@ func TestSchemaChange(t *testing.T) {
  onlineddl.CheckCancelAllMigrations(t, &vtParams, 1)
  })
  t.Run("unthrottle shard 1", func(t *testing.T) {
- _, body, err := unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
+ body, err := unthrottleApp(shards[1].Vttablets[0], onlineDDLThrottlerAppName)
  assert.NoError(t, err)
  assert.Contains(t, body, onlineDDLThrottlerAppName)
  })

go/test/endtoend/tabletgateway/buffer/buffer_test_helpers.go

@@ -288,7 +288,7 @@ func (bt *BufferingTest) Test(t *testing.T) {
  // Healthcheck interval on tablet is set to 1s, so sleep for 2s
  time.Sleep(2 * time.Second)
  conn, err := mysql.Connect(context.Background(), &vtParams)
- require.Nil(t, err)
+ require.NoError(t, err)
  defer conn.Close()
 
  // Insert two rows for the later threads (critical read, update).
@@ -350,11 +350,14 @@ func (bt *BufferingTest) Test(t *testing.T) {
  //At least one thread should have been buffered.
  //This may fail if a failover is too fast. Add retries then.
  resp, err := http.Get(clusterInstance.VtgateProcess.VerifyURL)
- require.Nil(t, err)
+ require.NoError(t, err)
+ defer resp.Body.Close()
+
  require.Equal(t, 200, resp.StatusCode)
 
  var metadata VTGateBufferingStats
- respByte, _ := io.ReadAll(resp.Body)
+ respByte, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
  err = json.Unmarshal(respByte, &metadata)
  require.NoError(t, err)
 

go/test/endtoend/tabletgateway/vtgate_test.go

@@ -45,7 +45,7 @@ func TestVtgateHealthCheck(t *testing.T) {
  verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
  ctx := context.Background()
  conn, err := mysql.Connect(ctx, &vtParams)
- require.Nil(t, err)
+ require.NoError(t, err)
  defer conn.Close()
 
  qr := utils.Exec(t, conn, "show vitess_tablets")
@@ -59,7 +59,7 @@ func TestVtgateReplicationStatusCheck(t *testing.T) {
  verifyVtgateVariables(t, clusterInstance.VtgateProcess.VerifyURL)
  ctx := context.Background()
  conn, err := mysql.Connect(ctx, &vtParams)
- require.Nil(t, err)
+ require.NoError(t, err)
  defer conn.Close()
 
  // Only returns rows for REPLICA and RDONLY tablets -- so should be 2 of them
@@ -72,10 +72,12 @@ func TestVtgateReplicationStatusCheck(t *testing.T) {
 func verifyVtgateVariables(t *testing.T, url string) {
  resp, err := http.Get(url)
  require.NoError(t, err)
+ defer resp.Body.Close()
  require.Equal(t, 200, resp.StatusCode, "Vtgate api url response not found")
 
  resultMap := make(map[string]any)
- respByte, _ := io.ReadAll(resp.Body)
+ respByte, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
  err = json.Unmarshal(respByte, &resultMap)
  require.NoError(t, err)
  assert.Contains(t, resultMap, "VtgateVSchemaCounts", "Vschema count should be present in variables")
@@ -203,7 +205,7 @@ func TestReplicaTransactions(t *testing.T) {
  // been restarted and the session lost
  replicaTablet.VttabletProcess.ServingStatus = "SERVING"
  err = replicaTablet.VttabletProcess.Setup()
- require.Nil(t, err)
+ require.NoError(t, err)
  serving := replicaTablet.VttabletProcess.WaitForStatus("SERVING", 60*time.Second)
  assert.Equal(t, serving, true, "Tablet did not become ready within a reasonable time")
  utils.AssertContainsError(t, readConn, fetchAllCustomers, "not found")