fix(pkg/sshd): wrap git receive functionality in a locking utility fu… (#339)

* fix(pkg/sshd): wrap git receive functionality in a locking utility function

The function always tries once to unlock

Fixes deis/builder#309
Doesn’t attempt to fix deis/builder#220

Author: arschles

pkg/sshd/lock.go

@@ -1,11 +1,16 @@
 package sshd
 
 import (
+	"errors"
 	"fmt"
 	"sync"
 	"time"
 )
 
+var (
+	errAlreadyLocked = errors.New("already locked")
+)
+
 // RepositoryLock interface that allows the creation of a lock associated
 // with a repository name to avoid simultaneous git operations.
 type RepositoryLock interface {
@@ -20,6 +25,14 @@ type RepositoryLock interface {
 	Unlock(repoName string, timeout time.Duration) error
 }
 
+func wrapInLock(lck RepositoryLock, repoName string, timeout time.Duration, fn func() error) error {
+	if err := lck.Lock(repoName, timeout); err != nil {
+		return errAlreadyLocked
+	}
+	defer lck.Unlock(repoName, timeout)
+	return fn()
+}
+
 // NewInMemoryRepositoryLock returns a new instance of a RepositoryLock.
 func NewInMemoryRepositoryLock() RepositoryLock {
 	return &inMemoryRepoLock{

pkg/sshd/lock_test.go

@@ -86,6 +86,17 @@ func TestDoubleLockUnlock(t *testing.T) {
 	}
 }
 
+func TestWrapInLock(t *testing.T) {
+	lck := NewInMemoryRepositoryLock()
+	assert.NoErr(t, wrapInLock(lck, "repo", 0*time.Second, func() error {
+		return nil
+	}))
+	lck.Lock("repo", 0*time.Second)
+	assert.Err(t, errAlreadyLocked, wrapInLock(lck, "repo", 0*time.Second, func() error {
+		return nil
+	}))
+}
+
 func lockAndCallback(rl RepositoryLock, id string, callbackCh chan<- interface{}) {
 	if err := rl.Lock(id, time.Duration(0)); err == nil {
 		callbackCh <- true

pkg/sshd/server.go

@@ -254,41 +254,20 @@ func (s *server) answer(channel ssh.Channel, requests <-chan *ssh.Request, conda
 					channel.Stderr().Write([]byte("No repo given"))
 					return err
 				}
-				if err = s.pushLock.Lock(repoName, time.Duration(0)); err != nil {
+				wrapErr := wrapInLock(s.pushLock, repoName, time.Duration(0), s.runReceive(req, sshconn, channel, repoName, parts, condata))
+				if wrapErr == errAlreadyLocked {
 					log.Info(multiplePush)
 					// The error must be in git format
-					if err := gitPktLine(channel, fmt.Sprintf("ERR %v\n", multiplePush)); err != nil {
+					if pktErr := gitPktLine(channel, fmt.Sprintf("ERR %v\n", multiplePush)); pktErr != nil {
 						log.Err("Failed to write to channel: %s", err)
 					}
 					sendExitStatus(1, channel)
 					req.Reply(false, nil)
 					return nil
 				}
 
-				req.Reply(true, nil) // We processed. Yay.
-				if !strings.Contains(sshconn.Permissions.Extensions["apps"], repoName) {
-					if err := s.pushLock.Unlock(repoName, time.Duration(0)); err != nil {
-						log.Err("unable to unlock repository lock for %s (%s)", repoName, err)
-						// TODO: this is an important error case that needs to be covered
-						// Probably the best solution is to change the lock into a lease so that even on unlock
-						// failures, RepositoryLock will eventually yield
-					}
-					return errBuildAppPerm
-				}
-				repo := repoName + ".git"
-				err = git.Receive(repo, parts[0], s.gitHome, channel, sshconn.Permissions.Extensions["fingerprint"], sshconn.Permissions.Extensions["user"], condata, s.receivetype)
-				if err != nil {
-					if err := s.pushLock.Unlock(repoName, time.Duration(0)); err != nil {
-						log.Err("unable to unlock repository lock for %s (%s)", repoName, err)
-						// TODO: this is an important error case that needs to be covered
-						// Probably the best solution is to change the lock into a lease so that even on unlock
-						// failures, RepositoryLock will eventually yield
-					}
-					return err
-				}
-
 				var xs uint32
-				if err != nil {
+				if wrapErr != nil {
 					log.Err("Failed git receive: %v", err)
 					xs = 1
 				}
@@ -316,6 +295,37 @@ func (s *server) answer(channel ssh.Channel, requests <-chan *ssh.Request, conda
 	return nil
 }
 
+func (s *server) runReceive(
+	req *ssh.Request,
+	sshConn *ssh.ServerConn,
+	channel ssh.Channel,
+	repoName string,
+	parts []string,
+	connData string,
+) func() error {
+	return func() error {
+		req.Reply(true, nil) // We processed. Yay.
+		if !strings.Contains(sshConn.Permissions.Extensions["apps"], repoName) {
+			return errBuildAppPerm
+		}
+		repo := repoName + ".git"
+		recvErr := git.Receive(
+			repo,
+			parts[0],
+			s.gitHome,
+			channel,
+			sshConn.Permissions.Extensions["fingerprint"],
+			sshConn.Permissions.Extensions["user"],
+			connData,
+			s.receivetype,
+		)
+		if recvErr != nil {
+			return recvErr
+		}
+		return nil
+	}
+}
+
 // ExecCmd is an SSH exec request.
 type ExecCmd struct {
 	Value string