Fixing config file lock mechanism

common/commands/config.go

@@ -2,26 +2,34 @@ package commands
 
 import (
 	"fmt"
+	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
+	"github.com/jfrog/jfrog-cli-core/v2/utils/ioutils"
+	"github.com/jfrog/jfrog-cli-core/v2/utils/lock"
 	"io/ioutil"
 	"reflect"
 	"strconv"
 	"strings"
 	"sync"
 
-	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
-	"github.com/jfrog/jfrog-cli-core/v2/utils/ioutils"
-
 	"github.com/jfrog/jfrog-client-go/auth"
 
 	"github.com/jfrog/jfrog-cli-core/v2/artifactory/utils"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/config"
-	"github.com/jfrog/jfrog-cli-core/v2/utils/lock"
 	clientutils "github.com/jfrog/jfrog-client-go/utils"
 	"github.com/jfrog/jfrog-client-go/utils/errorutils"
 	"github.com/jfrog/jfrog-client-go/utils/io/fileutils"
 	"github.com/jfrog/jfrog-client-go/utils/log"
 )
 
+type CmdType string
+
+const (
+	AddOrEdit CmdType = "AddOrEdit"
+	Delete    CmdType = "Delete"
+	Use       CmdType = "Use"
+	Clear     CmdType = "Clear"
+)
+
 // Internal golang locking for the same process.
 var mutex sync.Mutex
 
@@ -34,10 +42,11 @@ type ConfigCommand struct {
 	serverId         string
 	// For unit tests
 	disablePromptUrls bool
+	cmdType           CmdType
 }
 
-func NewConfigCommand() *ConfigCommand {
-	return &ConfigCommand{}
+func NewConfigCommand(cmdType CmdType, serverId string) *ConfigCommand {
+	return &ConfigCommand{cmdType: cmdType, serverId: serverId}
 }
 
 func (cc *ConfigCommand) SetServerId(serverId string) *ConfigCommand {
@@ -71,7 +80,34 @@ func (cc *ConfigCommand) SetDetails(details *config.ServerDetails) *ConfigComman
 }
 
 func (cc *ConfigCommand) Run() error {
-	return cc.Config()
+	mutex.Lock()
+	defer mutex.Unlock()
+	lockDirPath, err := coreutils.GetJfrogConfigLockDir()
+	if err != nil {
+		return err
+	}
+	lockFile, err := lock.CreateLock(lockDirPath)
+	defer func() {
+		e := lockFile.Unlock()
+		if err == nil {
+			err = e
+		}
+	}()
+	if err != nil {
+		return err
+	}
+	switch cc.cmdType {
+	case AddOrEdit:
+		return cc.config()
+	case Delete:
+		return cc.delete()
+	case Use:
+		return cc.use()
+	case Clear:
+		return cc.clear()
+	default:
+		return fmt.Errorf("Not supported config command type: " + string(cc.cmdType))
+	}
 }
 
 func (cc *ConfigCommand) ServerDetails() (*config.ServerDetails, error) {
@@ -90,20 +126,7 @@ func (cc *ConfigCommand) CommandName() string {
 	return "config"
 }
 
-func (cc *ConfigCommand) Config() error {
-	mutex.Lock()
-	defer mutex.Unlock()
-	lockDirPath, err := coreutils.GetJfrogConfigLockDir()
-	if err != nil {
-		return err
-	}
-	lockFile, err := lock.CreateLock(lockDirPath)
-	defer lockFile.Unlock()
-
-	if err != nil {
-		return err
-	}
-
+func (cc *ConfigCommand) config() error {
 	configurations, err := cc.prepareConfigurationData()
 	if err != nil {
 		return err
@@ -134,7 +157,7 @@ func (cc *ConfigCommand) Config() error {
 
 	// Artifactory expects the username to be lower-cased. In case it is not,
 	// Artifactory will silently save it lower-cased, but the token creation
-	// REST API will fail with a non lower-cased username.
+	// REST API will fail with a non-lower-cased username.
 	cc.details.User = strings.ToLower(cc.details.User)
 
 	if len(configurations) == 1 {
@@ -211,7 +234,7 @@ func (cc *ConfigCommand) prepareConfigurationData() ([]*config.ServerDetails, er
 	return configurations, err
 }
 
-/// Returning the first non empty value:
+/// Returning the first non-empty value:
 // 1. The serverId argument sent.
 // 2. details.ServerId
 // 3. defaultDetails.ServerId
@@ -388,7 +411,7 @@ func Import(serverToken string) error {
 		details:  serverDetails,
 		serverId: serverDetails.ServerId,
 	}
-	return configCommand.Config()
+	return configCommand.config()
 }
 
 func Export(serverName string) error {
@@ -435,14 +458,14 @@ func logIfNotEmpty(value, prefix string, mask bool) {
 	}
 }
 
-func DeleteConfig(serverName string) error {
+func (cc *ConfigCommand) delete() error {
 	configurations, err := config.GetAllServersConfigs()
 	if err != nil {
 		return err
 	}
 	var isDefault, isFoundName bool
 	for i, config := range configurations {
-		if config.ServerId == serverName {
+		if config.ServerId == cc.serverId {
 			isDefault = config.IsDefault
 			configurations = append(configurations[:i], configurations[i+1:]...)
 			isFoundName = true
@@ -456,20 +479,20 @@ func DeleteConfig(serverName string) error {
 	if isFoundName {
 		return config.SaveServersConf(configurations)
 	}
-	log.Info("\"" + serverName + "\" configuration could not be found.\n")
+	log.Info("\"" + cc.serverId + "\" configuration could not be found.\n")
 	return nil
 }
 
 // Set the default configuration
-func Use(serverId string) error {
+func (cc *ConfigCommand) use() error {
 	configurations, err := config.GetAllServersConfigs()
 	if err != nil {
 		return err
 	}
 	var serverFound *config.ServerDetails
 	newDefaultServer := true
 	for _, config := range configurations {
-		if config.ServerId == serverId {
+		if config.ServerId == cc.serverId {
 			serverFound = config
 			if config.IsDefault {
 				newDefaultServer = false
@@ -491,11 +514,11 @@ func Use(serverId string) error {
 		log.Info(fmt.Sprintf("Using server ID '%s' (%s).", serverFound.ServerId, serverFound.Url))
 		return nil
 	}
-	return errorutils.CheckErrorf("Could not find a server with ID '%s'.", serverId)
+	return errorutils.CheckErrorf("Could not find a server with ID '%s'.", cc.serverId)
 }
 
-func ClearConfig(interactive bool) error {
-	if interactive {
+func (cc *ConfigCommand) clear() error {
+	if cc.interactive {
 		confirmed := coreutils.AskYesNo("Are you sure you want to delete all the configurations?", false)
 		if !confirmed {
 			return nil

common/commands/config_test.go

@@ -122,13 +122,13 @@ func TestBasicAuthOnlyOption(t *testing.T) {
 	outputConfig, err := configAndGetTestServer(t, inputDetails, true, false)
 	assert.NoError(t, err)
 	assert.Equal(t, coreutils.TokenRefreshDisabled, outputConfig.TokenRefreshInterval, "expected refreshable token to be disabled")
-	assert.NoError(t, DeleteConfig("test"))
+	assert.NoError(t, NewConfigCommand(Delete, "test").Run())
 
 	// Verify setting the option enables refreshable tokens.
 	outputConfig, err = configAndGetTestServer(t, inputDetails, false, false)
 	assert.NoError(t, err)
 	assert.Equal(t, coreutils.TokenRefreshDefaultInterval, outputConfig.TokenRefreshInterval, "expected refreshable token to be enabled")
-	assert.NoError(t, DeleteConfig("test"))
+	assert.NoError(t, NewConfigCommand(Delete, "test").Run())
 }
 
 func testExportImport(t *testing.T, inputDetails *config.ServerDetails) {
@@ -143,14 +143,14 @@ func configAndTest(t *testing.T, inputDetails *config.ServerDetails, interactive
 	outputConfig, err := configAndGetTestServer(t, inputDetails, true, interactive)
 	assert.NoError(t, err)
 	assert.Equal(t, configStructToString(inputDetails), configStructToString(outputConfig), "unexpected configuration was saved to file")
-	assert.NoError(t, DeleteConfig("test"))
+	assert.NoError(t, NewConfigCommand(Delete, "test").Run())
 	testExportImport(t, inputDetails)
 }
 
 func configAndGetTestServer(t *testing.T, inputDetails *config.ServerDetails, basicAuthOnly, interactive bool) (*config.ServerDetails, error) {
-	configCmd := NewConfigCommand().SetDetails(inputDetails).SetServerId("test").SetUseBasicAuthOnly(basicAuthOnly).SetInteractive(interactive)
+	configCmd := NewConfigCommand(AddOrEdit, "test").SetDetails(inputDetails).SetUseBasicAuthOnly(basicAuthOnly).SetInteractive(interactive)
 	configCmd.disablePromptUrls = true
-	assert.NoError(t, configCmd.Config())
+	assert.NoError(t, configCmd.Run())
 	return GetConfig("test", false)
 }
 

general/envsetup/envsetup.go

@@ -175,11 +175,11 @@ func configServer(server *config.ServerDetails) error {
 	}
 	// Take the server name from host name: https://myjfrog.jfrog.com/ -> myjfrog
 	serverId := strings.Split(u.Host, ".")[0]
-	configCmd := commands.NewConfigCommand().SetInteractive(false).SetServerId(serverId).SetDetails(server)
-	if err = configCmd.Config(); err != nil {
+	configCmd := commands.NewConfigCommand(commands.AddOrEdit, serverId).SetInteractive(false).SetDetails(server)
+	if err = configCmd.Run(); err != nil {
 		return err
 	}
-	return commands.Use(serverId)
+	return commands.NewConfigCommand(commands.Use, serverId).SetInteractive(false).SetDetails(server).Run()
 }
 
 type myJfrogGetStatusRequest struct {

utils/config/config.go

@@ -4,26 +4,25 @@ import (
 	"bytes"
 	"encoding/json"
 	"errors"
-	accessAuth "github.com/jfrog/jfrog-client-go/access/auth"
-	pipelinesAuth "github.com/jfrog/jfrog-client-go/pipelines/auth"
-	"io/ioutil"
-	"os"
-	"path/filepath"
-	"strconv"
-	"strings"
-	"time"
-
 	"github.com/buger/jsonparser"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	cliLog "github.com/jfrog/jfrog-cli-core/v2/utils/log"
+	accessAuth "github.com/jfrog/jfrog-client-go/access/auth"
 	artifactoryAuth "github.com/jfrog/jfrog-client-go/artifactory/auth"
 	"github.com/jfrog/jfrog-client-go/auth"
 	distributionAuth "github.com/jfrog/jfrog-client-go/distribution/auth"
+	pipelinesAuth "github.com/jfrog/jfrog-client-go/pipelines/auth"
 	"github.com/jfrog/jfrog-client-go/utils"
 	"github.com/jfrog/jfrog-client-go/utils/errorutils"
 	"github.com/jfrog/jfrog-client-go/utils/io/fileutils"
 	"github.com/jfrog/jfrog-client-go/utils/log"
 	xrayAuth "github.com/jfrog/jfrog-client-go/xray/auth"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"time"
 )
 
 func init() {

utils/lock/lock.go

@@ -38,7 +38,6 @@ func (locks Locks) Less(i, j int) bool {
 
 // Creating a new lock object.
 func (lock *Lock) createNewLockFile(lockDirPath string) error {
-	lock.currentTime = time.Now().UnixNano()
 	err := fileutils.CreateDirIfNotExist(lockDirPath)
 	if err != nil {
 		return err
@@ -50,13 +49,17 @@ func (lock *Lock) createNewLockFile(lockDirPath string) error {
 
 func (lock *Lock) createFile(folderName string, pid int) error {
 	// We are creating an empty file with the pid and current time part of the name
-	lock.fileName = filepath.Join(folderName, "jfrog-cli.conf.lck."+strconv.Itoa(pid)+"."+strconv.FormatInt(lock.currentTime, 10))
+	lock.fileName = filepath.Join(folderName, "jfrog-cli.conf.lck."+strconv.Itoa(pid)+"."+strconv.FormatInt(time.Now().UnixNano(), 10))
 	log.Debug("Creating lock file: ", lock.fileName)
 	file, err := os.OpenFile(lock.fileName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0666)
 	if err != nil {
 		return errorutils.CheckError(err)
 	}
-
+	fileInfo, err := file.Stat()
+	if err != nil {
+		return errorutils.CheckError(err)
+	}
+	lock.currentTime = fileInfo.ModTime().UnixNano()
 	if err = file.Close(); err != nil {
 		return errorutils.CheckError(err)
 	}
@@ -85,7 +88,7 @@ func (lock *Lock) lock() error {
 		// If the first timestamp in the sorted locks slice is equal to this timestamp
 		// means that the lock can be acquired
 		if locks[0].currentTime == lock.currentTime {
-			// Edge case, if at the same time (by the nano seconds) two different process created two files.
+			// Edge case, if at the same time (by the nanoseconds) two different process created two files.
 			// We are checking the PID to know which process can run.
 			if locks[0].pid != lock.pid {
 				err := lock.removeOtherLockOrWait(locks[0], &filesList)
@@ -163,23 +166,20 @@ func (lock *Lock) getLocks(filesList []string) (Locks, error) {
 	// Slice of all the timestamps that currently the lock directory has
 	var files Locks
 	for _, path := range filesList {
-		fileName := filepath.Base(path)
-		splitted := strings.Split(fileName, ".")
-
-		if len(splitted) != 5 {
-			return nil, errorutils.CheckErrorf("Failed while parsing the file name: %s located at: %s. Expecting a different format.", fileName, path)
-		}
-		// Last element is the timestamp.
-		time, err := strconv.ParseInt(splitted[4], 10, 64)
+		fileInfo, err := os.Stat(path)
 		if err != nil {
 			return nil, errorutils.CheckError(err)
 		}
+		splitted := strings.Split(fileInfo.Name(), ".")
+		if len(splitted) != 5 {
+			return nil, errorutils.CheckErrorf("Failed while parsing the file name: %s located at: %s. Expecting a different format.", fileInfo.Name(), path)
+		}
 		pid, err := strconv.Atoi(splitted[3])
 		if err != nil {
 			return nil, errorutils.CheckError(err)
 		}
 		file := Lock{
-			currentTime: time,
+			currentTime: fileInfo.ModTime().UnixNano(),
 			pid:         pid,
 			fileName:    path,
 		}

utils/lock/lock_test.go

@@ -10,7 +10,6 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
-	"time"
 )
 
 var testLockDirPath string
@@ -180,11 +179,7 @@ func TestCreateFile(t *testing.T) {
 }
 
 func getLock(pid int, t *testing.T) (Lock, string) {
-	currentTime := time.Now().UnixNano()
-	lock := Lock{
-		pid:         pid,
-		currentTime: currentTime,
-	}
+	lock := Lock{pid: pid}
 	assert.NotZero(t, testLockDirPath, "An error occurred while initializing testLockDirPath")
 	assert.NoError(t, fileutils.CreateDirIfNotExist(testLockDirPath))
 	err := lock.createFile(testLockDirPath, pid)