From 2e0309a4af4aa338062e52d9ac97a8a1a2ca73d7 Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Thu, 1 Aug 2024 15:53:52 +0200 Subject: [PATCH 1/5] Introduce mutex policy to the v2 tools and use it to write the installed.json --- v2/pkgs/tools.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/v2/pkgs/tools.go b/v2/pkgs/tools.go index 14f853e9..d4f62a86 100644 --- a/v2/pkgs/tools.go +++ b/v2/pkgs/tools.go @@ -29,6 +29,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "github.com/arduino/arduino-create-agent/gen/tools" "github.com/arduino/arduino-create-agent/index" @@ -60,6 +61,7 @@ type Tools struct { index *index.Resource folder string behaviour string + mutex sync.RWMutex } // New will return a Tool object, allowing the caller to execute operations on it. @@ -70,6 +72,7 @@ func New(index *index.Resource, folder, behaviour string) *Tools { index: index, folder: folder, behaviour: behaviour, + mutex: sync.RWMutex{}, } } @@ -187,7 +190,7 @@ func (t *Tools) Install(ctx context.Context, payload *tools.ToolPayload) (*tools } if ok && pathExists(location) { // overwrite the default tool with this one - err := writeInstalled(t.folder, path) + err := t.writeInstalled(path) if err != nil { return nil, err } @@ -245,7 +248,7 @@ func (t *Tools) install(ctx context.Context, path, url, checksum string) (*tools } // Write installed.json for retrocompatibility with v1 - err = writeInstalled(t.folder, path) + err = t.writeInstalled(path) if err != nil { return nil, err } @@ -311,9 +314,11 @@ func checkInstalled(folder, key string) (string, bool, error) { return location, ok, err } -func writeInstalled(folder, path string) error { +func (t *Tools) writeInstalled(path string) error { + t.mutex.RLock() + defer t.mutex.RUnlock() // read installed.json - installedFile, err := utilities.SafeJoin(folder, "installed.json") + installedFile, err := utilities.SafeJoin(t.folder, "installed.json") if err != nil { return err } @@ -325,7 +330,7 @@ func writeInstalled(folder, path string) error { parts := strings.Split(path, string(filepath.Separator)) tool := parts[len(parts)-2] toolWithVersion := fmt.Sprint(tool, "-", parts[len(parts)-1]) - toolFile, err := utilities.SafeJoin(folder, path) + toolFile, err := utilities.SafeJoin(t.folder, path) if err != nil { return err } From b2d1aa0a56b5b1f0deb247a46e19d17659e2dce5 Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Thu, 1 Aug 2024 16:06:03 +0200 Subject: [PATCH 2/5] Use a map to store installed.json information --- v2/pkgs/tools.go | 58 ++++++++++++++++++------------------------------ 1 file changed, 22 insertions(+), 36 deletions(-) diff --git a/v2/pkgs/tools.go b/v2/pkgs/tools.go index d4f62a86..5ff2906e 100644 --- a/v2/pkgs/tools.go +++ b/v2/pkgs/tools.go @@ -61,6 +61,7 @@ type Tools struct { index *index.Resource folder string behaviour string + installed map[string]string mutex sync.RWMutex } @@ -68,12 +69,15 @@ type Tools struct { // The New function will accept an index as parameter (used to download the indexes) // and a folder used to download the indexes func New(index *index.Resource, folder, behaviour string) *Tools { - return &Tools{ + t := &Tools{ index: index, folder: folder, behaviour: behaviour, + installed: map[string]string{}, mutex: sync.RWMutex{}, } + t.readInstalled() + return t } // Installedhead is here only because it was required by the front-end. @@ -184,10 +188,7 @@ func (t *Tools) Install(ctx context.Context, payload *tools.ToolPayload) (*tools key := correctTool.Name + "-" + correctTool.Version // Check if it already exists if t.behaviour == "keep" && pathExists(t.folder) { - location, ok, err := checkInstalled(t.folder, key) - if err != nil { - return nil, err - } + location, ok := t.installed[key] if ok && pathExists(location) { // overwrite the default tool with this one err := t.writeInstalled(path) @@ -288,44 +289,24 @@ func rename(base string) extract.Renamer { } } -func readInstalled(installedFile string) (map[string]string, error) { +func (t *Tools) readInstalled() error { + t.mutex.Lock() + defer t.mutex.Unlock() // read installed.json - installed := map[string]string{} - data, err := os.ReadFile(installedFile) - if err == nil { - err = json.Unmarshal(data, &installed) - if err != nil { - return nil, err - } - } - return installed, nil -} - -func checkInstalled(folder, key string) (string, bool, error) { - installedFile, err := utilities.SafeJoin(folder, "installed.json") + installedFile, err := utilities.SafeJoin(t.folder, "installed.json") if err != nil { - return "", false, err + return err } - installed, err := readInstalled(installedFile) + data, err := os.ReadFile(installedFile) if err != nil { - return "", false, err + return err } - location, ok := installed[key] - return location, ok, err + return json.Unmarshal(data, &t.installed) } func (t *Tools) writeInstalled(path string) error { t.mutex.RLock() defer t.mutex.RUnlock() - // read installed.json - installedFile, err := utilities.SafeJoin(t.folder, "installed.json") - if err != nil { - return err - } - installed, err := readInstalled(installedFile) - if err != nil { - return err - } parts := strings.Split(path, string(filepath.Separator)) tool := parts[len(parts)-2] @@ -334,10 +315,15 @@ func (t *Tools) writeInstalled(path string) error { if err != nil { return err } - installed[tool] = toolFile - installed[toolWithVersion] = toolFile + t.installed[tool] = toolFile + t.installed[toolWithVersion] = toolFile - data, err := json.Marshal(installed) + data, err := json.Marshal(t.installed) + if err != nil { + return err + } + + installedFile, err := utilities.SafeJoin(t.folder, "installed.json") if err != nil { return err } From 1a36a2bb610c3906e30f3430fd20aacd70c6b503 Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Mon, 5 Aug 2024 09:38:24 +0200 Subject: [PATCH 3/5] Use the correct lock for reading and writing --- v2/pkgs/tools.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/v2/pkgs/tools.go b/v2/pkgs/tools.go index 5ff2906e..67198689 100644 --- a/v2/pkgs/tools.go +++ b/v2/pkgs/tools.go @@ -290,8 +290,8 @@ func rename(base string) extract.Renamer { } func (t *Tools) readInstalled() error { - t.mutex.Lock() - defer t.mutex.Unlock() + t.mutex.RLock() + defer t.mutex.RUnlock() // read installed.json installedFile, err := utilities.SafeJoin(t.folder, "installed.json") if err != nil { @@ -305,8 +305,8 @@ func (t *Tools) readInstalled() error { } func (t *Tools) writeInstalled(path string) error { - t.mutex.RLock() - defer t.mutex.RUnlock() + t.mutex.Lock() + defer t.mutex.Unlock() parts := strings.Split(path, string(filepath.Separator)) tool := parts[len(parts)-2] From 650622b5bd6e784a69ebd4fc2606e9c25b02ddbb Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Mon, 5 Aug 2024 14:20:20 +0200 Subject: [PATCH 4/5] Include the v2.Tools constructor in the old Tools struct --- tools/download.go | 5 ++--- tools/tools.go | 3 +++ v2/pkgs/tools.go | 5 +++++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/tools/download.go b/tools/download.go index 6e5fa8b7..8c4a37a6 100644 --- a/tools/download.go +++ b/tools/download.go @@ -25,7 +25,6 @@ import ( "github.com/arduino/arduino-create-agent/gen/tools" "github.com/arduino/arduino-create-agent/utilities" - "github.com/arduino/arduino-create-agent/v2/pkgs" ) // Download will parse the index at the indexURL for the tool to download. @@ -45,8 +44,8 @@ import ( // if it already exists. func (t *Tools) Download(pack, name, version, behaviour string) error { - tool := pkgs.New(t.index, t.directory.String(), behaviour) - _, err := tool.Install(context.Background(), &tools.ToolPayload{Name: name, Version: version, Packager: pack}) + t.tools.SetBehaviour(behaviour) + _, err := t.tools.Install(context.Background(), &tools.ToolPayload{Name: name, Version: version, Packager: pack}) if err != nil { return err } diff --git a/tools/tools.go b/tools/tools.go index cb9efc78..5cecc508 100644 --- a/tools/tools.go +++ b/tools/tools.go @@ -22,6 +22,7 @@ import ( "sync" "github.com/arduino/arduino-create-agent/index" + "github.com/arduino/arduino-create-agent/v2/pkgs" "github.com/arduino/go-paths-helper" "github.com/xrash/smetrics" ) @@ -47,6 +48,7 @@ type Tools struct { logger func(msg string) installed map[string]string mutex sync.RWMutex + tools *pkgs.Tools } // New will return a Tool object, allowing the caller to execute operations on it. @@ -60,6 +62,7 @@ func New(directory *paths.Path, index *index.Resource, logger func(msg string)) logger: logger, installed: map[string]string{}, mutex: sync.RWMutex{}, + tools: pkgs.New(index, directory.String(), "replace"), } _ = t.readMap() return t diff --git a/v2/pkgs/tools.go b/v2/pkgs/tools.go index 67198689..c84d207f 100644 --- a/v2/pkgs/tools.go +++ b/v2/pkgs/tools.go @@ -331,6 +331,11 @@ func (t *Tools) writeInstalled(path string) error { return os.WriteFile(installedFile, data, 0644) } +// SetBehaviour sets the download behaviour to either keep or replace +func (t *Tools) SetBehaviour(behaviour string) { + t.behaviour = behaviour +} + func pathExists(path string) bool { _, err := os.Stat(path) if err == nil { From 33f59b544056dacf9597f885b64b091a4f2d049c Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Mon, 5 Aug 2024 16:27:36 +0200 Subject: [PATCH 5/5] Add corrupted json test --- tools/download_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tools/download_test.go b/tools/download_test.go index 8863a09d..7cf2fab0 100644 --- a/tools/download_test.go +++ b/tools/download_test.go @@ -160,3 +160,23 @@ func TestDownload(t *testing.T) { }) } } + +func TestCorruptedInstalled(t *testing.T) { + // prepare the test environment + tempDir := t.TempDir() + tempDirPath := paths.New(tempDir) + testIndex := index.Resource{ + IndexFile: *paths.New("testdata", "test_tool_index.json"), + LastRefresh: time.Now(), + } + corruptedJSON := tempDirPath.Join("installed.json") + fileJSON, err := corruptedJSON.Create() + require.NoError(t, err) + defer fileJSON.Close() + _, err = fileJSON.Write([]byte("Hello")) + require.NoError(t, err) + testTools := New(tempDirPath, &testIndex, func(msg string) { t.Log(msg) }) + // Download the tool + err = testTools.Download("arduino-test", "avrdude", "6.3.0-arduino17", "keep") + require.NoError(t, err) +}