[lxc-devel] [lxd/master] Fix race in cancel code and make golint clean

[stgraber on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 301 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20180216/3112c6bd/attachment.bin>
-------------- next part --------------
From a32f083d562f4981204d7c8a624281390cfec90e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Fri, 16 Feb 2018 14:39:52 -0500
Subject: [PATCH 1/2] shared/cancel: Properly lock map
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 shared/cancel/canceler.go | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/shared/cancel/canceler.go b/shared/cancel/canceler.go
index 6a27347a0..44059981d 100644
--- a/shared/cancel/canceler.go
+++ b/shared/cancel/canceler.go
@@ -3,21 +3,31 @@ package cancel
 import (
 	"fmt"
 	"net/http"
+	"sync"
 )
 
 // A struct to track canceleation
 type Canceler struct {
 	reqChCancel map[*http.Request]chan struct{}
+	lock        sync.Mutex
 }
 
 func NewCanceler() *Canceler {
 	c := Canceler{}
+
+	c.lock.Lock()
 	c.reqChCancel = make(map[*http.Request]chan struct{})
+	c.lock.Unlock()
+
 	return &c
 }
 
 func (c *Canceler) Cancelable() bool {
-	return len(c.reqChCancel) > 0
+	c.lock.Lock()
+	length := len(c.reqChCancel)
+	c.lock.Unlock()
+
+	return length > 0
 }
 
 func (c *Canceler) Cancel() error {
@@ -25,10 +35,13 @@ func (c *Canceler) Cancel() error {
 		return fmt.Errorf("This operation cannot be canceled at this time")
 	}
 
+	c.lock.Lock()
 	for req, ch := range c.reqChCancel {
 		close(ch)
 		delete(c.reqChCancel, req)
 	}
+	c.lock.Unlock()
+
 	return nil
 }
 
@@ -36,14 +49,18 @@ func CancelableDownload(c *Canceler, client *http.Client, req *http.Request) (*h
 	chDone := make(chan bool)
 	chCancel := make(chan struct{})
 	if c != nil {
+		c.lock.Lock()
 		c.reqChCancel[req] = chCancel
+		c.lock.Unlock()
 	}
 	req.Cancel = chCancel
 
 	go func() {
 		<-chDone
 		if c != nil {
+			c.lock.Lock()
 			delete(c.reqChCancel, req)
+			c.lock.Unlock()
 		}
 	}()
 

From c4ef9a68cb62dbc70cacf440708bd9096197ce5e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phane=20Graber?= <stgraber at ubuntu.com>
Date: Fri, 16 Feb 2018 14:43:30 -0500
Subject: [PATCH 2/2] shared/cancel: Make the cancel code golint clean
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Stéphane Graber <stgraber at ubuntu.com>
---
 shared/cancel/canceler.go      | 8 ++++++--
 test/suites/static_analysis.sh | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/shared/cancel/canceler.go b/shared/cancel/canceler.go
index 44059981d..b3356cf37 100644
--- a/shared/cancel/canceler.go
+++ b/shared/cancel/canceler.go
@@ -6,12 +6,13 @@ import (
 	"sync"
 )
 
-// A struct to track canceleation
+// Canceler tracks a cancelable operation
 type Canceler struct {
 	reqChCancel map[*http.Request]chan struct{}
 	lock        sync.Mutex
 }
 
+// NewCanceler returns a new Canceler struct
 func NewCanceler() *Canceler {
 	c := Canceler{}
 
@@ -22,6 +23,7 @@ func NewCanceler() *Canceler {
 	return &c
 }
 
+// Cancelable indicates whether there are operations that support cancelation
 func (c *Canceler) Cancelable() bool {
 	c.lock.Lock()
 	length := len(c.reqChCancel)
@@ -30,9 +32,10 @@ func (c *Canceler) Cancelable() bool {
 	return length > 0
 }
 
+// Cancel will attempt to cancel all ongoing operations
 func (c *Canceler) Cancel() error {
 	if !c.Cancelable() {
-		return fmt.Errorf("This operation cannot be canceled at this time")
+		return fmt.Errorf("This operation can't be canceled at this time")
 	}
 
 	c.lock.Lock()
@@ -45,6 +48,7 @@ func (c *Canceler) Cancel() error {
 	return nil
 }
 
+// CancelableDownload performs an http request and allows for it to be canceled at any time
 func CancelableDownload(c *Canceler, client *http.Client, req *http.Request) (*http.Response, chan bool, error) {
 	chDone := make(chan bool)
 	chCancel := make(chan struct{})
diff --git a/test/suites/static_analysis.sh b/test/suites/static_analysis.sh
index 81b8dd67f..425287a42 100644
--- a/test/suites/static_analysis.sh
+++ b/test/suites/static_analysis.sh
@@ -58,6 +58,7 @@ test_static_analysis() {
       golint -set_exit_status client/
       golint -set_exit_status lxc/config/
       golint -set_exit_status shared/api/
+      golint -set_exit_status shared/cancel/
       golint -set_exit_status shared/cmd/
       golint -set_exit_status shared/gnuflag/
       golint -set_exit_status shared/i18n/