Skip to content

Commit

Permalink
fix(language-server): Fix race conditions when using internal proxy c…
Browse files Browse the repository at this point in the history
…oncurrently (#5332)

* fix: Fix race conditions when using internal proxy concurrently

* fix: incorrect log message

* fix: extend cleanup to remove tmp folders of old processes

* fix: fix logic and make pattern more explicit
  • Loading branch information
PeterSchafer authored Jun 20, 2024
1 parent 6620964 commit e5d35b4
Show file tree
Hide file tree
Showing 5 changed files with 122 additions and 10 deletions.
7 changes: 6 additions & 1 deletion cliv2/cmd/cliv2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,9 @@ func displayError(err error, userInterface ui.UserInterface, config configuratio
}

uiError := userInterface.OutputError(err)
globalLogger.Err(uiError).Msg("ui failed show error")
if uiError != nil {
globalLogger.Err(uiError).Msg("ui failed to show error")
}
}
}
}
Expand Down Expand Up @@ -573,6 +575,9 @@ func MainWithErrorCode() int {
cliAnalytics.GetInstrumentation().SetStatus(analytics.Failure)
}

// cleanup resources in use
basic_workflows.Cleanup()

return exitCode
}

Expand Down
69 changes: 69 additions & 0 deletions cliv2/internal/cliv2/cliv2.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,12 @@ import (
"os"
"os/exec"
"path"
"path/filepath"
"regexp"
"slices"
"strconv"
"strings"
"syscall"
"time"

"github.com/gofrs/flock"
Expand Down Expand Up @@ -135,6 +138,16 @@ func (c *CLI) Init() (err error) {
}

func (c *CLI) ClearCache() error {
err := c.clearVersionFolders()
if err != nil {
return err
}

err = c.clearTemporaryProcessFolders()
return err
}

func (c *CLI) clearVersionFolders() error {
// Get files in directory
fileInfo, err := os.ReadDir(c.CacheDirectory)
if err != nil {
Expand Down Expand Up @@ -163,6 +176,58 @@ func (c *CLI) ClearCache() error {
return nil
}

func (c *CLI) clearTemporaryProcessFolders() error {
// clean up the tmp dir of the current version
maxConsecutiveDeletes := 5
deleteCount := 0
tempDir := filepath.Dir(c.GetTempDir())
fileInfo, err := os.ReadDir(tempDir)
if err != nil {
return err
}

// cleanup tmp files related to a non-existing process
processTempPattern := regexp.MustCompile("pid([0-9]*)")
for _, file := range fileInfo {
currentPath := path.Join(tempDir, file.Name())
matches := processTempPattern.FindStringSubmatch(file.Name())
if len(matches) == 2 {
processFound := true
pid, localError := strconv.Atoi(matches[1])
if localError != nil {
continue
}

p, localError := os.FindProcess(pid)
if localError != nil {
processFound = false
}

if p != nil {
localError = p.Signal(syscall.Signal(0))
if localError != nil {
processFound = false
}
}

if !processFound {
deleteCount++
err = os.RemoveAll(currentPath)
if err != nil {
c.DebugLogger.Println("Error deleting temporary files: ", currentPath)
}
}
}

// Stop the loop after 5 deletions to not create too much overhead
if deleteCount == maxConsecutiveDeletes {
break
}
}

return nil
}

func (c *CLI) AppendEnvironmentVariables(env []string) {
c.env = append(c.env, env...)
}
Expand Down Expand Up @@ -210,6 +275,10 @@ func (c *CLI) GetBinaryLocation() string {
return c.v1BinaryLocation
}

func (c *CLI) GetTempDir() string {
return local_utils.GetTemporaryDirectory(c.CacheDirectory, cliv1.CLIV1Version())
}

func (c *CLI) printVersion() {
_, _ = fmt.Fprintln(c.stdout, GetFullVersion())
}
Expand Down
16 changes: 12 additions & 4 deletions cliv2/internal/cliv2/cliv2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"os/exec"
"path"
"path/filepath"
"runtime"
"sort"
"testing"
Expand Down Expand Up @@ -410,11 +411,16 @@ func Test_clearCache(t *testing.T) {
lockfile := path.Join(cli.CacheDirectory, "v1.914.0.lock")
randomFile := path.Join(versionNoV, "filename")
currentVersion := cli.GetBinaryLocation()
tempDir := filepath.Dir(cli.GetTempDir())
oldProcessTempDir := path.Join(tempDir, "pid123")
oldProcessTempDirFile := path.Join(oldProcessTempDir, "bla.txt")

_ = os.Mkdir(versionWithV, 0755)
_ = os.Mkdir(versionNoV, 0755)
_ = os.WriteFile(randomFile, []byte("Writing some strings"), 0666)
_ = os.WriteFile(lockfile, []byte("Writing some strings"), 0666)
assert.NoError(t, os.Mkdir(versionWithV, 0755))
assert.NoError(t, os.Mkdir(versionNoV, 0755))
assert.NoError(t, os.Mkdir(oldProcessTempDir, 0755))
assert.NoError(t, os.WriteFile(randomFile, []byte("Writing some strings"), 0666))
assert.NoError(t, os.WriteFile(lockfile, []byte("Writing some strings"), 0666))
assert.NoError(t, os.WriteFile(oldProcessTempDirFile, []byte("Writing some strings"), 0666))

// clear cache
err := cli.ClearCache()
Expand All @@ -424,9 +430,11 @@ func Test_clearCache(t *testing.T) {
assert.NoDirExists(t, versionWithV)
assert.NoDirExists(t, versionNoV)
assert.NoFileExists(t, randomFile)
assert.NoFileExists(t, oldProcessTempDirFile)
// check if directories that need to exist still exist
assert.FileExists(t, currentVersion)
assert.FileExists(t, lockfile)
assert.DirExists(t, cli.GetTempDir())
}

func Test_clearCacheBigCache(t *testing.T) {
Expand Down
4 changes: 3 additions & 1 deletion cliv2/internal/utils/directories.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package utils

import (
"fmt"
"os"
"path"

Expand All @@ -15,7 +16,8 @@ const CACHEDIR_PERMISSION = 0755
// |- Temp directory (example: /Users/username/Library/Caches/snyk/snyk-cli/1.1075.0/tmp/)

func GetTemporaryDirectory(baseCacheDirectory string, versionNumber string) string {
return path.Join(GetVersionCacheDirectory(baseCacheDirectory, versionNumber), "tmp")
pid := os.Getpid()
return path.Join(GetVersionCacheDirectory(baseCacheDirectory, versionNumber), "tmp", fmt.Sprintf("pid%d", pid))
}

func GetVersionCacheDirectory(baseCacheDirectory string, versionNumber string) string {
Expand Down
36 changes: 32 additions & 4 deletions cliv2/pkg/basic_workflows/legacycli.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"bytes"
"net/http"
"os"
"sync"

"github.com/pkg/errors"
"github.com/rs/zerolog"
"github.com/snyk/go-application-framework/pkg/auth"
"github.com/snyk/go-application-framework/pkg/configuration"
"github.com/snyk/go-application-framework/pkg/logging"
Expand All @@ -23,6 +25,9 @@ import (
var WORKFLOWID_LEGACY_CLI workflow.Identifier = workflow.NewWorkflowIdentifier("legacycli")
var DATATYPEID_LEGACY_CLI_STDOUT workflow.Identifier = workflow.NewTypeIdentifier(WORKFLOWID_LEGACY_CLI, "stdout")

var proxySingleton *proxy.WrapperProxy
var proxyMutex sync.Mutex

const (
PROXY_NOAUTH string = "proxy-noauth"
)
Expand Down Expand Up @@ -132,12 +137,10 @@ func legacycliWorkflow(
cli.SetIoStreams(os.Stdin, os.Stdout, scrubbedStderr)
}

// init proxy object
wrapperProxy, err := proxy.NewWrapperProxy(config, cliv2.GetFullVersion(), debugLogger)
wrapperProxy, err := getProxyInstance(config, debugLogger)
if err != nil {
return output, errors.Wrap(err, "Failed to create proxy!")
return output, err
}
defer wrapperProxy.Close()

wrapperProxy.SetUpstreamProxyAuthentication(proxyAuthenticationMechanism)

Expand Down Expand Up @@ -175,3 +178,28 @@ func legacycliWorkflow(

return output, err
}

func Cleanup() {
proxyMutex.Lock()
defer proxyMutex.Unlock()
if proxySingleton != nil {
proxySingleton.Close()
proxySingleton = nil
}
}

func getProxyInstance(config configuration.Configuration, debugLogger *zerolog.Logger) (*proxy.WrapperProxy, error) {
var err error
proxyMutex.Lock()
defer proxyMutex.Unlock()

if proxySingleton == nil {
// init proxy object
proxySingleton, err = proxy.NewWrapperProxy(config, cliv2.GetFullVersion(), debugLogger)
if err != nil {
return nil, errors.Wrap(err, "Failed to create proxy!")
}
}

return proxySingleton, nil
}

0 comments on commit e5d35b4

Please sign in to comment.