Merge pull request #1579 from weaveworks/1576-cancel-publish-on-stop

probe: add timeout to publisher and cancel requests on Stop()

probe/appclient/app_client.go

@@ -19,9 +19,10 @@ import (
 )
 
 const (
-	initialBackoff = 1 * time.Second
-	maxBackoff     = 60 * time.Second
-	dialTimeout    = 5 * time.Second
+	httpClientTimeout = 4 * time.Second
+	initialBackoff    = 1 * time.Second
+	maxBackoff        = 60 * time.Second
+	dialTimeout       = 5 * time.Second
 )
 
 // AppClient is a client to an app for dealing with controls.
@@ -76,6 +77,7 @@ func NewAppClient(pc ProbeConfig, hostname, target string, control xfer.ControlH
 		target:      target,
 		client: http.Client{
 			Transport: httpTransport,
+			Timeout:   httpClientTimeout,
 		},
 		wsDialer: websocket.Dialer{
 			TLSClientConfig: httpTransport.TLSClientConfig,
@@ -245,6 +247,10 @@ func (c *appClient) publish(r io.Reader) error {
 	req.Header.Set("Content-Encoding", "gzip")
 	req.Header.Set("Content-Type", "application/msgpack")
 	// req.Header.Set("Content-Type", "application/binary") // TODO: we should use http.DetectContentType(..) on the gob'ed
+
+	// Make sure this request is cancelled when we stop the client
+	req.Cancel = c.quit
+
 	resp, err := c.client.Do(req)
 	if err != nil {
 		return err

probe/appclient/app_client_internal_test.go

@@ -173,3 +173,59 @@ func TestAppClientDetails(t *testing.T) {
 		return
 	}
 }
+
+// Make sure Stopping a client works even if the connection or the remote app
+// gets stuck for whatever reason.
+// See https://github.com/weaveworks/scope/issues/1576
+func TestStop(t *testing.T) {
+	var (
+		rpt            = report.MakeReport()
+		stopHanging    = make(chan struct{})
+		receivedReport = make(chan struct{})
+	)
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		close(receivedReport)
+		<-stopHanging
+	})
+
+	s := httptest.NewServer(handlers.CompressHandler(handler))
+	defer s.Close()
+
+	u, err := url.Parse(s.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	pc := ProbeConfig{
+		Token:    "",
+		ProbeID:  "",
+		Insecure: false,
+	}
+
+	p, err := NewAppClient(pc, u.Host, s.URL, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	rp := NewReportPublisher(p)
+
+	// Make sure the app received our report and is stuck
+	for done := false; !done; {
+		select {
+		case <-receivedReport:
+			done = true
+		default:
+			if err := rp.Publish(rpt); err != nil {
+				t.Error(err)
+			}
+			time.Sleep(10 * time.Millisecond)
+		}
+	}
+
+	// Close the client while the app is stuck
+	p.Stop()
+
+	// Let the server go so that the test can end
+	close(stopHanging)
+}