Merge pull request #1154 from tamird/remove-unnecessary-once

gps: simplify shutdown cleanup

Author: sdboyer

internal/gps/source_manager.go

@@ -275,42 +275,22 @@ func (sm *SourceMgr) HandleSignals(sigch chan os.Signal) {
 	// Run a new goroutine with the input sigch and the fresh qch
 	go func(sch chan os.Signal, qch <-chan struct{}) {
 		defer signal.Stop(sch)
-		for {
-			select {
-			case <-sch:
-				// Set up a timer to uninstall the signal handler after three
-				// seconds, so that the user can easily force termination with a
-				// second ctrl-c
-				go func(c <-chan time.Time) {
-					<-c
-					signal.Stop(sch)
-				}(time.After(3 * time.Second))
-
-				if !atomic.CompareAndSwapInt32(&sm.releasing, 0, 1) {
-					// Something's already called Release() on this sm, so we
-					// don't have to do anything, as we'd just be redoing
-					// that work. Instead, deregister and return.
-					return
-				}
-
-				opc := sm.suprvsr.count()
-				if opc > 0 {
-					fmt.Printf("Signal received: waiting for %v ops to complete...\n", opc)
-				}
-
-				// Mutex interaction in a signal handler is, as a general rule,
-				// unsafe. I'm not clear on whether the guarantees Go provides
-				// around signal handling, or having passed this through a
-				// channel in general, obviate those concerns, but it's a lot
-				// easier to just rely on the mutex contained in the Once right
-				// now, so do that until it proves problematic or someone
-				// provides a clear explanation.
-				sm.relonce.Do(func() { sm.doRelease() })
-				return
-			case <-qch:
-				// quit channel triggered - deregister our sigch and return
-				return
+		select {
+		case <-sch:
+			// Set up a timer to uninstall the signal handler after three
+			// seconds, so that the user can easily force termination with a
+			// second ctrl-c
+			time.AfterFunc(3*time.Second, func() {
+				signal.Stop(sch)
+			})
+
+			if opc := sm.suprvsr.count(); opc > 0 {
+				fmt.Printf("Signal received: waiting for %v ops to complete...\n", opc)
 			}
+
+			sm.Release()
+		case <-qch:
+			// quit channel triggered - deregister our sigch and return
 		}
 	}(sigch, sm.qch)
 	// Try to ensure handler is blocked in for-select before releasing the mutex
@@ -349,46 +329,34 @@ func (e CouldNotCreateLockError) Error() string {
 // longer safe to call methods against it; all method calls will immediately
 // result in errors.
 func (sm *SourceMgr) Release() {
-	// Set sm.releasing before entering the Once func to guarantee that no
-	// _more_ method calls will stack up if/while waiting.
-	atomic.CompareAndSwapInt32(&sm.releasing, 0, 1)
+	atomic.StoreInt32(&sm.releasing, 1)
 
-	// Whether 'releasing' is set or not, we don't want this function to return
-	// until after the doRelease process is done, as doing so could cause the
-	// process to terminate before a signal-driven doRelease() call has a chance
-	// to finish its cleanup.
-	sm.relonce.Do(sm.doRelease)
-}
+	sm.relonce.Do(func() {
+		// Send the signal to the supervisor to cancel all running calls.
+		sm.cancelAll()
+		sm.suprvsr.wait()
 
-// doRelease actually releases physical resources (files on disk, etc.).
-//
-// This must be called only and exactly once. Calls to it should be wrapped in
-// the sm.relonce sync.Once instance.
-func (sm *SourceMgr) doRelease() {
-	// Send the signal to the supervisor to cancel all running calls
-	sm.cancelAll()
-	sm.suprvsr.wait()
-
-	// Close the source coordinator.
-	sm.srcCoord.close()
-
-	// Close the file handle for the lock file and remove it from disk
-	sm.lf.Unlock()
-	os.Remove(filepath.Join(sm.cachedir, "sm.lock"))
-
-	// Close the qch, if non-nil, so the signal handlers run out. This will
-	// also deregister the sig channel, if any has been set up.
-	if sm.qch != nil {
-		close(sm.qch)
-	}
+		// Close the source coordinator.
+		sm.srcCoord.close()
+
+		// Close the file handle for the lock file and remove it from disk
+		sm.lf.Unlock()
+		os.Remove(filepath.Join(sm.cachedir, "sm.lock"))
+
+		// Close the qch, if non-nil, so the signal handlers run out. This will
+		// also deregister the sig channel, if any has been set up.
+		if sm.qch != nil {
+			close(sm.qch)
+		}
+	})
 }
 
 // GetManifestAndLock returns manifest and lock information for the provided
 // ProjectIdentifier, at the provided Version. The work of producing the
 // manifest and lock is delegated to the provided ProjectAnalyzer's
 // DeriveManifestAndLock() method.
 func (sm *SourceMgr) GetManifestAndLock(id ProjectIdentifier, v Version, an ProjectAnalyzer) (Manifest, Lock, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return nil, nil, smIsReleased{}
 	}
 
@@ -403,7 +371,7 @@ func (sm *SourceMgr) GetManifestAndLock(id ProjectIdentifier, v Version, an Proj
 // ListPackages parses the tree of the Go packages at and below the ProjectRoot
 // of the given ProjectIdentifier, at the given version.
 func (sm *SourceMgr) ListPackages(id ProjectIdentifier, v Version) (pkgtree.PackageTree, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return pkgtree.PackageTree{}, smIsReleased{}
 	}
 
@@ -428,7 +396,7 @@ func (sm *SourceMgr) ListPackages(id ProjectIdentifier, v Version) (pkgtree.Pack
 // is not accessible (network outage, access issues, or the resource actually
 // went away), an error will be returned.
 func (sm *SourceMgr) ListVersions(id ProjectIdentifier) ([]PairedVersion, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return nil, smIsReleased{}
 	}
 
@@ -444,7 +412,7 @@ func (sm *SourceMgr) ListVersions(id ProjectIdentifier) ([]PairedVersion, error)
 // RevisionPresentIn indicates whether the provided Revision is present in the given
 // repository.
 func (sm *SourceMgr) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return false, smIsReleased{}
 	}
 
@@ -460,7 +428,7 @@ func (sm *SourceMgr) RevisionPresentIn(id ProjectIdentifier, r Revision) (bool,
 // SourceExists checks if a repository exists, either upstream or in the cache,
 // for the provided ProjectIdentifier.
 func (sm *SourceMgr) SourceExists(id ProjectIdentifier) (bool, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return false, smIsReleased{}
 	}
 
@@ -478,7 +446,7 @@ func (sm *SourceMgr) SourceExists(id ProjectIdentifier) (bool, error) {
 //
 // The primary use case for this is prefetching.
 func (sm *SourceMgr) SyncSourceFor(id ProjectIdentifier) error {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return smIsReleased{}
 	}
 
@@ -493,7 +461,7 @@ func (sm *SourceMgr) SyncSourceFor(id ProjectIdentifier) error {
 // ExportProject writes out the tree of the provided ProjectIdentifier's
 // ProjectRoot, at the provided version, to the provided directory.
 func (sm *SourceMgr) ExportProject(id ProjectIdentifier, v Version, to string) error {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return smIsReleased{}
 	}
 
@@ -513,7 +481,7 @@ func (sm *SourceMgr) ExportProject(id ProjectIdentifier, v Version, to string) e
 // paths. (A special exception is written for gopkg.in to minimize network
 // activity, as its behavior is well-structured)
 func (sm *SourceMgr) DeduceProjectRoot(ip string) (ProjectRoot, error) {
-	if atomic.CompareAndSwapInt32(&sm.releasing, 1, 1) {
+	if atomic.LoadInt32(&sm.releasing) == 1 {
 		return "", smIsReleased{}
 	}