Add TryLockWithRetry in favor of TryLock

Author: Fabianoshz

server/events/post_workflow_hooks_command_runner.go

@@ -52,7 +52,7 @@ func (w *DefaultPostWorkflowHooksCommandRunner) RunPostHooks(ctx *command.Contex
 
 	ctx.Log.Info("Post-workflow hooks configured, running...")
 
-	unlockFn, err := w.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, DefaultWorkspace, DefaultRepoRelDir)
+	unlockFn, err := w.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, DefaultWorkspace, DefaultRepoRelDir, false, false)
 	if err != nil {
 		return err
 	}

server/events/pre_workflow_hooks_command_runner.go

@@ -52,7 +52,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(ctx *command.Context,
 
 	ctx.Log.Info("Pre-workflow hooks configured, running...")
 
-	unlockFn, err := w.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, DefaultWorkspace, DefaultRepoRelDir)
+	unlockFn, err := w.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, DefaultWorkspace, DefaultRepoRelDir, false, false)
 	if err != nil {
 		return err
 	}

server/events/project_command_builder.go

@@ -389,7 +389,7 @@ func (p *DefaultProjectCommandBuilder) buildAllCommandsByCfg(ctx *command.Contex
 	// Need to lock the workspace we're about to clone to.
 	workspace := DefaultWorkspace
 
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir, false, false)
 	if err != nil {
 		ctx.Log.Warn("workspace was locked")
 		return nil, err
@@ -587,7 +587,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont
 	var pcc []command.ProjectContext
 
 	ctx.Log.Debug("building plan command")
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir, false, false)
 	if err != nil {
 		return pcc, err
 	}
@@ -806,7 +806,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectCommand(ctx *command.Context,
 	}
 
 	var projCtx []command.ProjectContext
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, workspace, DefaultRepoRelDir, false, false)
 	if err != nil {
 		return projCtx, err
 	}

server/events/project_command_runner.go

@@ -354,7 +354,7 @@ func (p *DefaultProjectCommandRunner) doApprovePolicies(ctx command.ProjectConte
 	ctx.Log.Debug("acquired lock for project")
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return nil, "", err
 	}
@@ -458,7 +458,7 @@ func (p *DefaultProjectCommandRunner) doPolicyCheck(ctx command.ProjectContext)
 	// Acquire internal lock for the directory we're going to operate in.
 	// We should refactor this to keep the lock for the duration of plan and policy check since as of now
 	// there is a small gap where we don't have the lock and if we can't get this here, we should just unlock the PR.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return nil, "", err
 	}
@@ -574,7 +574,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 	ctx.Log.Debug("acquired lock for project")
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return nil, "", err
 	}
@@ -651,7 +651,7 @@ func (p *DefaultProjectCommandRunner) doApply(ctx command.ProjectContext) (apply
 	ctx.Log.Debug("acquired lock for project")
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return "", "", err
 	}
@@ -689,7 +689,7 @@ func (p *DefaultProjectCommandRunner) doVersion(ctx command.ProjectContext) (ver
 	}
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return "", "", err
 	}
@@ -730,7 +730,7 @@ func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out
 	ctx.Log.Debug("acquired lock for project")
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return nil, "", err
 	}
@@ -771,7 +771,7 @@ func (p *DefaultProjectCommandRunner) doStateRm(ctx command.ProjectContext) (out
 	ctx.Log.Debug("acquired lock for project")
 
 	// Acquire internal lock for the directory we're going to operate in.
-	unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir)
+	unlockFn, err := p.WorkingDirLocker.TryLockWithRetry(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, ctx.Workspace, ctx.RepoRelDir, false, false)
 	if err != nil {
 		return nil, "", err
 	}

server/events/working_dir_locker.go

@@ -28,11 +28,14 @@ import (
 // on disk and we haven't written Atlantis (yet) to handle concurrent execution
 // within this workspace.
 type WorkingDirLocker interface {
-	// TryLock tries to acquire a lock for this repo, pull, workspace, and path.
+	// TryLockWithRetry tries to acquire a lock for this repo, pull, workspace, and path.
 	// It returns a function that should be used to unlock the workspace and
 	// an error if the workspace is already locked. The error is expected to
 	// be printed to the pull request.
-	TryLock(repoFullName string, pullNum int, workspace string, path string) (func(), error)
+	// The caller can define if the function should automatically retry to acquire the lock
+	// if either the pull request or the workspace is locked already.
+	TryLockWithRetry(repoFullName string, pullNum int, workspace string, path string, retryWorkspaceLocked bool, retryPullLocked bool) (func(), error)
+
 	// TryLockPull tries to acquire a lock for all the workspaces in this repo
 	// and pull.
 	// It returns a function that should be used to unlock the workspace and
@@ -56,8 +59,8 @@ type DefaultWorkingDirLocker struct {
 
 // NewDefaultWorkingDirLocker is a constructor.
 func NewDefaultWorkingDirLocker(lockAcquireTimeoutSeconds int) *DefaultWorkingDirLocker {
-	if lockAcquireTimeoutSeconds < 1 {
-		lockAcquireTimeoutSeconds = 1
+	if lockAcquireTimeoutSeconds < 0 {
+		lockAcquireTimeoutSeconds = 0
 	}
 
 	return &DefaultWorkingDirLocker{
@@ -83,7 +86,7 @@ func (d *DefaultWorkingDirLocker) TryLockPull(repoFullName string, pullNum int)
 	}, nil
 }
 
-func (d *DefaultWorkingDirLocker) TryLock(repoFullName string, pullNum int, workspace string, path string) (func(), error) {
+func (d *DefaultWorkingDirLocker) TryLockWithRetry(repoFullName string, pullNum int, workspace string, path string, retryWorkspaceLocked bool, retryPullLocked bool) (func(), error) {
 	ticker := time.NewTicker(500 * time.Millisecond)
 	timeout := time.NewTimer(time.Duration(d.lockAcquireTimeoutSeconds) * time.Second)
 
@@ -97,8 +100,21 @@ func (d *DefaultWorkingDirLocker) TryLock(repoFullName string, pullNum int, work
 				" command that is running for this pull request.\n"+
 				"Wait until the previous command is complete and try again", workspace, path)
 		case <-ticker.C:
-			lockAcquired := d.tryAcquireLock(pullKey, workspaceKey)
-			if lockAcquired {
+			pullInUse, workspaceInUse := d.tryAcquireLock(pullKey, workspaceKey)
+
+			if pullInUse && !retryPullLocked {
+				return func() {}, fmt.Errorf("the %s workspace at path %s is currently locked by another"+
+					" command that is running for this pull request.\n"+
+					"Wait until the previous command is complete and try again", workspace, path)
+			}
+
+			if workspaceInUse && !retryWorkspaceLocked{
+				return func() {}, fmt.Errorf("the %s workspace at path %s is currently locked by another"+
+					" command that is running for this pull request.\n"+
+					"Wait until the previous command is complete and try again", workspace, path)
+			}
+
+			if !workspaceInUse && !pullInUse {
 				return func() {
 					d.unlock(repoFullName, pullNum, workspace, path)
 				}, nil
@@ -107,22 +123,27 @@ func (d *DefaultWorkingDirLocker) TryLock(repoFullName string, pullNum int, work
 	}
 }
 
-func (d *DefaultWorkingDirLocker) tryAcquireLock(pullKey string, workspaceKey string) bool {
+func (d *DefaultWorkingDirLocker) tryAcquireLock(pullKey string, workspaceKey string) (bool, bool) {
 	d.mutex.Lock()
 	defer d.mutex.Unlock()
 
-	acquireLock := true
+	pullInUse := false
+	workspaceInUse := false
 	for _, l := range d.locks {
-		if l == pullKey || l == workspaceKey {
-			acquireLock = false
+		if l == pullKey {
+			pullInUse = true
+		}
+
+		if l == workspaceKey {
+			workspaceInUse = true
 		}
 	}
 
-	if acquireLock {
+	if !pullInUse && !workspaceInUse {
 		d.locks = append(d.locks, workspaceKey)
 	}
 
-	return acquireLock
+	return pullInUse, workspaceInUse
 }
 
 // Unlock unlocks the workspace for this pull.