Cache SSH connections

The underlying SSH connections are kept open and are reused
across several SSH sessions. This is due to upstream issues in
which concurrent/parallel SSH connections may lead to instability.

golang/go#51926
golang/go#27140
Signed-off-by: Paulo Gomes <[email protected]>

Author: Paulo Gomes

pkg/git/libgit2/managed/http.go

@@ -288,11 +288,13 @@ func (self *httpSmartSubtransportStream) Free() {
 	if self.resp != nil {
 		traceLog.Info("[http]: httpSmartSubtransportStream.Free()")
 
-		// ensure body is fully processed and closed
-		// for increased likelihood of transport reuse in HTTP/1.x.
-		// it should not be a problem to do this more than once.
-		io.Copy(io.Discard, self.resp.Body)
-		self.resp.Body.Close()
+		if self.resp.Body != nil {
+			// ensure body is fully processed and closed
+			// for increased likelihood of transport reuse in HTTP/1.x.
+			// it should not be a problem to do this more than once.
+			_, _ = io.Copy(io.Discard, self.resp.Body) // errors can be safely ignored
+			_ = self.resp.Body.Close()                 // errors can be safely ignored
+		}
 	}
 }
 
@@ -366,8 +368,11 @@ func (self *httpSmartSubtransportStream) sendRequest() error {
 		if req.Method == "POST" && resp.StatusCode >= 301 && resp.StatusCode <= 308 {
 			// ensure body is fully processed and closed
 			// for increased likelihood of transport reuse in HTTP/1.x.
-			io.Copy(io.Discard, resp.Body)
-			resp.Body.Close()
+			_, _ = io.Copy(io.Discard, resp.Body) // errors can be safely ignored
+
+			if err := resp.Body.Close(); err != nil {
+				return err
+			}
 
 			// The next try will go against the new destination
 			self.req.URL, err = resp.Location()
@@ -386,8 +391,10 @@ func (self *httpSmartSubtransportStream) sendRequest() error {
 
 		// ensure body is fully processed and closed
 		// for increased likelihood of transport reuse in HTTP/1.x.
-		io.Copy(io.Discard, resp.Body)
-		resp.Body.Close()
+		_, _ = io.Copy(io.Discard, resp.Body) // errors can be safely ignored
+		if err := resp.Body.Close(); err != nil {
+			return err
+		}
 
 		return fmt.Errorf("Unhandled HTTP error %s", resp.Status)
 	}

pkg/git/libgit2/managed/managed_test.go

@@ -36,7 +36,7 @@ import (
 
 func TestHttpAction_CreateClientRequest(t *testing.T) {
 	tests := []struct {
-		description    string
+		name           string
 		url            string
 		expectedUrl    string
 		expectedMethod string
@@ -46,7 +46,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 		wantedErr      error
 	}{
 		{
-			description:    "Uploadpack: no changes when no options found",
+			name:           "Uploadpack: no changes when no options found",
 			url:            "https://sometarget/abc",
 			expectedUrl:    "https://sometarget/abc/git-upload-pack",
 			expectedMethod: "POST",
@@ -56,7 +56,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 			wantedErr:      nil,
 		},
 		{
-			description:    "UploadpackLs: no changes when no options found",
+			name:           "UploadpackLs: no changes when no options found",
 			url:            "https://sometarget/abc",
 			expectedUrl:    "https://sometarget/abc/info/refs?service=git-upload-pack",
 			expectedMethod: "GET",
@@ -66,7 +66,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 			wantedErr:      nil,
 		},
 		{
-			description:    "Receivepack: no changes when no options found",
+			name:           "Receivepack: no changes when no options found",
 			url:            "https://sometarget/abc",
 			expectedUrl:    "https://sometarget/abc/git-receive-pack",
 			expectedMethod: "POST",
@@ -76,7 +76,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 			wantedErr:      nil,
 		},
 		{
-			description:    "ReceivepackLs: no changes when no options found",
+			name:           "ReceivepackLs: no changes when no options found",
 			url:            "https://sometarget/abc",
 			expectedUrl:    "https://sometarget/abc/info/refs?service=git-receive-pack",
 			expectedMethod: "GET",
@@ -86,7 +86,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 			wantedErr:      nil,
 		},
 		{
-			description:    "override URL via options",
+			name:           "override URL via options",
 			url:            "https://initial-target/abc",
 			expectedUrl:    "https://final-target/git-upload-pack",
 			expectedMethod: "POST",
@@ -98,7 +98,7 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 			wantedErr: nil,
 		},
 		{
-			description:    "error when no http.transport provided",
+			name:           "error when no http.transport provided",
 			url:            "https://initial-target/abc",
 			expectedUrl:    "",
 			expectedMethod: "",
@@ -110,45 +110,47 @@ func TestHttpAction_CreateClientRequest(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		if tt.opts != nil {
-			AddTransportOptions(tt.url, *tt.opts)
-		}
-
-		_, req, err := createClientRequest(tt.url, tt.action, tt.transport)
-		if tt.wantedErr != nil {
-			if tt.wantedErr.Error() != err.Error() {
-				t.Errorf("%s: wanted: %v got: %v", tt.description, tt.wantedErr, err)
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.opts != nil {
+				AddTransportOptions(tt.url, *tt.opts)
 			}
-		} else {
-			assert.Equal(t, req.URL.String(), tt.expectedUrl)
-			assert.Equal(t, req.Method, tt.expectedMethod)
-		}
-
-		if tt.opts != nil {
-			RemoveTransportOptions(tt.url)
-		}
+
+			_, req, err := createClientRequest(tt.url, tt.action, tt.transport)
+			if tt.wantedErr != nil {
+				if tt.wantedErr.Error() != err.Error() {
+					t.Errorf("wanted: %v got: %v", tt.wantedErr, err)
+				}
+			} else {
+				assert.Equal(t, req.URL.String(), tt.expectedUrl)
+				assert.Equal(t, req.Method, tt.expectedMethod)
+			}
+
+			if tt.opts != nil {
+				RemoveTransportOptions(tt.url)
+			}
+		})
 	}
 }
 
 func TestOptions(t *testing.T) {
 	tests := []struct {
-		description  string
+		name         string
 		registerOpts bool
 		url          string
 		opts         TransportOptions
 		expectOpts   bool
 		expectedOpts *TransportOptions
 	}{
 		{
-			description:  "return registered option",
+			name:         "return registered option",
 			registerOpts: true,
 			url:          "https://target/?123",
 			opts:         TransportOptions{},
 			expectOpts:   true,
 			expectedOpts: &TransportOptions{},
 		},
 		{
-			description:  "match registered options",
+			name:         "match registered options",
 			registerOpts: true,
 			url:          "https://target/?876",
 			opts: TransportOptions{
@@ -162,7 +164,7 @@ func TestOptions(t *testing.T) {
 			},
 		},
 		{
-			description:  "ignore when options not registered",
+			name:         "ignore when options not registered",
 			registerOpts: false,
 			url:          "",
 			opts:         TransportOptions{},
@@ -172,28 +174,30 @@ func TestOptions(t *testing.T) {
 	}
 
 	for _, tt := range tests {
-		if tt.registerOpts {
-			AddTransportOptions(tt.url, tt.opts)
-		}
-
-		opts, found := transportOptions(tt.url)
-		if tt.expectOpts != found {
-			t.Errorf("%s: wanted %v got %v", tt.description, tt.expectOpts, found)
-		}
-
-		if tt.expectOpts {
-			if reflect.DeepEqual(opts, *tt.expectedOpts) {
-				t.Errorf("%s: wanted %v got %v", tt.description, *tt.expectedOpts, opts)
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.registerOpts {
+				AddTransportOptions(tt.url, tt.opts)
+			}
+
+			opts, found := transportOptions(tt.url)
+			if tt.expectOpts != found {
+				t.Errorf("%s: wanted %v got %v", tt.name, tt.expectOpts, found)
 			}
-		}
 
-		if tt.registerOpts {
-			RemoveTransportOptions(tt.url)
-		}
+			if tt.expectOpts {
+				if reflect.DeepEqual(opts, *tt.expectedOpts) {
+					t.Errorf("%s: wanted %v got %v", tt.name, *tt.expectedOpts, opts)
+				}
+			}
 
-		if _, found = transportOptions(tt.url); found {
-			t.Errorf("%s: option for %s was not removed", tt.description, tt.url)
-		}
+			if tt.registerOpts {
+				RemoveTransportOptions(tt.url)
+			}
+
+			if _, found = transportOptions(tt.url); found {
+				t.Errorf("%s: option for %s was not removed", tt.name, tt.url)
+			}
+		})
 	}
 }