Fix concurrent access to libraries manager gRPC functions. (#2480)

* Removed libraries index fields from LibraryManager

The presence of the Index* fileds inside LibraryManager didn't give any
functional benefit.

* Removed all references to LibraryManger.Index

* Removed unused tmp file creation

* downloadLibrary do not require a libmanager but just the downloadDir

* Inline method LibrariesManager.Install

* Move initializations closer to usage

* Use LibraryManager.FindAllInstalled instead of direct access to field

* Made LibraryManager.Libraries private

* Simplified libraries resolver init

* Made LibrariesManager.LibrariesDir private

* Removed DownloadsDir field from LibraryManager

* Removed librariesindex.Reference structure and related helpers

It does make things more complicated without any actual benefit.

* Removed state-altering methods from LibrariesManager

The original LibrariresManager has been split into three objects with
specific goals:
* The Builder object is used to construct a new LibrariesManager. It has
  methods to add librarires directories and to build the LibrariesManager.
* The Explorer object is used to query the LibrariesManager about
  installed libraries.
* The Installer object is used to rescan installed libraries and to
  install and uninstall.

* Fixed test

Author: cmaglie

commands/core/download.go

@@ -36,7 +36,7 @@ func PlatformDownload(ctx context.Context, req *rpc.PlatformDownloadRequest, dow
 	}
 	defer release()
 
-	version, err := commands.ParseVersion(req)
+	version, err := commands.ParseVersion(req.GetVersion())
 	if err != nil {
 		return nil, &cmderrors.InvalidVersionError{Cause: err}
 	}

commands/core/install.go

@@ -35,7 +35,7 @@ func PlatformInstall(ctx context.Context, req *rpc.PlatformInstallRequest, downl
 		}
 		defer release()
 
-		version, err := commands.ParseVersion(req)
+		version, err := commands.ParseVersion(req.GetVersion())
 		if err != nil {
 			return &cmderrors.InvalidVersionError{Cause: err}
 		}

commands/instances.go

@@ -301,17 +301,13 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 	}
 
 	// Create library manager and add libraries directories
-	lm := librariesmanager.NewLibraryManager(
-		pme.IndexDir,
-		pme.DownloadDir,
-	)
-	_ = instances.SetLibraryManager(instance, lm) // should never fail
+	lmb := librariesmanager.NewBuilder()
 
 	// Load libraries
 	for _, pack := range pme.GetPackages() {
 		for _, platform := range pack.Platforms {
 			if platformRelease := pme.GetInstalledPlatformRelease(platform); platformRelease != nil {
-				lm.AddLibrariesDir(&librariesmanager.LibrariesDir{
+				lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
 					PlatformRelease: platformRelease,
 					Path:            platformRelease.GetLibrariesDir(),
 					Location:        libraries.PlatformBuiltIn,
@@ -320,22 +316,33 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 		}
 	}
 
-	if err := lm.LoadIndex(); err != nil {
+	indexFileName, err := globals.LibrariesIndexResource.IndexFileName()
+	if err != nil {
+		// should never happen
+		panic("failed getting libraries index file name: " + err.Error())
+	}
+	indexFile := pme.IndexDir.Join(indexFileName)
+
+	logrus.WithField("index", indexFile).Info("Loading libraries index file")
+	li, err := librariesindex.LoadIndex(indexFile)
+	if err != nil {
 		s := status.Newf(codes.FailedPrecondition, tr("Loading index file: %v"), err)
 		responseError(s)
+		li = librariesindex.EmptyIndex
 	}
+	instances.SetLibrariesIndex(instance, li)
 
 	if profile == nil {
 		// Add directories of libraries bundled with IDE
 		if bundledLibsDir := configuration.IDEBuiltinLibrariesDir(configuration.Settings); bundledLibsDir != nil {
-			lm.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
 				Path:     bundledLibsDir,
 				Location: libraries.IDEBuiltIn,
 			})
 		}
 
 		// Add libraries directory from config file
-		lm.AddLibrariesDir(&librariesmanager.LibrariesDir{
+		lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
 			Path:     configuration.LibrariesDir(configuration.Settings),
 			Location: libraries.User,
 		})
@@ -349,17 +356,14 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 			if !libDir.IsDir() {
 				// Download library
 				taskCallback(&rpc.TaskProgress{Name: tr("Downloading library %s", libraryRef)})
-				libRelease := lm.Index.FindRelease(&librariesindex.Reference{
-					Name:    libraryRef.Library,
-					Version: libraryRef.Version,
-				})
-				if libRelease == nil {
+				libRelease, err := li.FindRelease(libraryRef.Library, libraryRef.Version)
+				if err != nil {
 					taskCallback(&rpc.TaskProgress{Name: tr("Library %s not found", libraryRef)})
 					err := &cmderrors.LibraryNotFoundError{Library: libraryRef.Library}
 					responseError(err.ToRPCStatus())
 					continue
 				}
-				if err := libRelease.Resource.Download(lm.DownloadsDir, nil, libRelease.String(), downloadCallback, ""); err != nil {
+				if err := libRelease.Resource.Download(pme.DownloadDir, nil, libRelease.String(), downloadCallback, ""); err != nil {
 					taskCallback(&rpc.TaskProgress{Name: tr("Error downloading library %s", libraryRef)})
 					e := &cmderrors.FailedLibraryInstallError{Cause: err}
 					responseError(e.ToRPCStatus())
@@ -369,7 +373,7 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 
 				// Install library
 				taskCallback(&rpc.TaskProgress{Name: tr("Installing library %s", libraryRef)})
-				if err := libRelease.Resource.Install(lm.DownloadsDir, libRoot, libDir); err != nil {
+				if err := libRelease.Resource.Install(pme.DownloadDir, libRoot, libDir); err != nil {
 					taskCallback(&rpc.TaskProgress{Name: tr("Error installing library %s", libraryRef)})
 					e := &cmderrors.FailedLibraryInstallError{Cause: err}
 					responseError(e.ToRPCStatus())
@@ -378,16 +382,23 @@ func Init(req *rpc.InitRequest, responseCallback func(r *rpc.InitResponse)) erro
 				taskCallback(&rpc.TaskProgress{Completed: true})
 			}
 
-			lm.AddLibrariesDir(&librariesmanager.LibrariesDir{
+			lmb.AddLibrariesDir(&librariesmanager.LibrariesDir{
 				Path:     libRoot,
 				Location: libraries.User,
 			})
 		}
 	}
 
-	for _, status := range lm.RescanLibraries() {
-		logrus.WithError(status.Err()).Warnf("Error loading library")
-		// TODO: report as warning: responseError(err)
+	lm := lmb.Build()
+	_ = instances.SetLibraryManager(instance, lm) // should never fail
+
+	{
+		lmi, release := lm.NewInstaller()
+		for _, status := range lmi.RescanLibraries() {
+			logrus.WithError(status.Err()).Warnf("Error loading library")
+			// TODO: report as warning: responseError(err)
+		}
+		release()
 	}
 
 	// Refreshes the locale used, this will change the
@@ -409,23 +420,18 @@ func Destroy(ctx context.Context, req *rpc.DestroyRequest) (*rpc.DestroyResponse
 // UpdateLibrariesIndex updates the library_index.json
 func UpdateLibrariesIndex(ctx context.Context, req *rpc.UpdateLibrariesIndexRequest, downloadCB rpc.DownloadProgressCB) error {
 	logrus.Info("Updating libraries index")
-	lm, err := instances.GetLibraryManager(req.GetInstance())
+	pme, release, err := instances.GetPackageManagerExplorer(req.GetInstance())
 	if err != nil {
 		return err
 	}
+	indexDir := pme.IndexDir
+	release()
 
-	if err := lm.IndexFile.Parent().MkdirAll(); err != nil {
+	if err := indexDir.MkdirAll(); err != nil {
 		return &cmderrors.PermissionDeniedError{Message: tr("Could not create index directory"), Cause: err}
 	}
 
-	// Create a temp dir to stage all downloads
-	tmp, err := paths.MkTempDir("", "library_index_download")
-	if err != nil {
-		return &cmderrors.TempDirCreationFailedError{Cause: err}
-	}
-	defer tmp.RemoveAll()
-
-	if err := globals.LibrariesIndexResource.Download(lm.IndexFile.Parent(), downloadCB); err != nil {
+	if err := globals.LibrariesIndexResource.Download(indexDir, downloadCB); err != nil {
 		return err
 	}
 

commands/internal/instances/instances.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/arduino/arduino-cli/commands/cmderrors"
 	"github.com/arduino/arduino-cli/internal/arduino/cores/packagemanager"
+	"github.com/arduino/arduino-cli/internal/arduino/libraries/librariesindex"
 	"github.com/arduino/arduino-cli/internal/arduino/libraries/librariesmanager"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
 	"github.com/arduino/arduino-cli/version"
@@ -17,6 +18,7 @@ import (
 type coreInstance struct {
 	pm *packagemanager.PackageManager
 	lm *librariesmanager.LibrariesManager
+	li *librariesindex.Index
 }
 
 // instances contains all the running Arduino Core Services instances
@@ -60,6 +62,49 @@ func GetLibraryManager(inst *rpc.Instance) (*librariesmanager.LibrariesManager,
 	return i.lm, nil
 }
 
+// GetLibraryManagerExplorer returns the library manager Explorer for the given instance.
+func GetLibraryManagerExplorer(inst *rpc.Instance) (*librariesmanager.Explorer, func(), error) {
+	lm, err := GetLibraryManager(inst)
+	if err != nil {
+		return nil, nil, err
+	}
+	lmi, release := lm.NewExplorer()
+	return lmi, release, nil
+}
+
+// GetLibraryManagerInstaller returns the library manager Installer for the given instance.
+func GetLibraryManagerInstaller(inst *rpc.Instance) (*librariesmanager.Installer, func(), error) {
+	lm, err := GetLibraryManager(inst)
+	if err != nil {
+		return nil, nil, err
+	}
+	lmi, release := lm.NewInstaller()
+	return lmi, release, nil
+}
+
+// GetLibrariesIndex returns the library index for the given instance.
+func GetLibrariesIndex(inst *rpc.Instance) (*librariesindex.Index, error) {
+	instancesMux.Lock()
+	defer instancesMux.Unlock()
+	i := instances[inst.GetId()]
+	if i == nil {
+		return nil, &cmderrors.InvalidInstanceError{}
+	}
+	return i.li, nil
+}
+
+// SetLibrariesIndex sets the library index for the given instance.
+func SetLibrariesIndex(inst *rpc.Instance, li *librariesindex.Index) error {
+	instancesMux.Lock()
+	defer instancesMux.Unlock()
+	i := instances[inst.GetId()]
+	if i == nil {
+		return &cmderrors.InvalidInstanceError{}
+	}
+	i.li = li
+	return nil
+}
+
 // SetLibraryManager sets the library manager for the given instance.
 func SetLibraryManager(inst *rpc.Instance, lm *librariesmanager.LibrariesManager) bool {
 	instancesMux.Lock()
@@ -74,16 +119,18 @@ func SetLibraryManager(inst *rpc.Instance, lm *librariesmanager.LibrariesManager
 
 // Create a new *rpc.Instance ready to be initialized
 func Create(dataDir, packagesDir, downloadsDir *paths.Path, extraUserAgent ...string) (*rpc.Instance, error) {
-	instance := &coreInstance{}
-
 	// Create package manager
 	userAgent := "arduino-cli/" + version.VersionInfo.VersionString
 	for _, ua := range extraUserAgent {
 		userAgent += " " + ua
 	}
 	tempDir := dataDir.Join("tmp")
-	instance.pm = packagemanager.NewBuilder(dataDir, packagesDir, downloadsDir, tempDir, userAgent).Build()
-	instance.lm = librariesmanager.NewLibraryManager(dataDir, downloadsDir)
+
+	instance := &coreInstance{
+		pm: packagemanager.NewBuilder(dataDir, packagesDir, downloadsDir, tempDir, userAgent).Build(),
+		lm: librariesmanager.NewBuilder().Build(),
+		li: librariesindex.EmptyIndex,
+	}
 
 	// Save instance
 	instancesMux.Lock()

commands/lib/download.go

@@ -18,13 +18,14 @@ package lib
 import (
 	"context"
 
+	"github.com/arduino/arduino-cli/commands"
 	"github.com/arduino/arduino-cli/commands/cmderrors"
 	"github.com/arduino/arduino-cli/commands/internal/instances"
 	"github.com/arduino/arduino-cli/internal/arduino/httpclient"
 	"github.com/arduino/arduino-cli/internal/arduino/libraries/librariesindex"
-	"github.com/arduino/arduino-cli/internal/arduino/libraries/librariesmanager"
 	"github.com/arduino/arduino-cli/internal/i18n"
 	rpc "github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"github.com/arduino/go-paths-helper"
 	"github.com/sirupsen/logrus"
 )
 
@@ -35,34 +36,47 @@ var tr = i18n.Tr
 func LibraryDownload(ctx context.Context, req *rpc.LibraryDownloadRequest, downloadCB rpc.DownloadProgressCB) (*rpc.LibraryDownloadResponse, error) {
 	logrus.Info("Executing `arduino-cli lib download`")
 
-	lm, err := instances.GetLibraryManager(req.GetInstance())
+	var downloadsDir *paths.Path
+	if pme, release, err := instances.GetPackageManagerExplorer(req.GetInstance()); err != nil {
+		return nil, err
+	} else {
+		downloadsDir = pme.DownloadDir
+		release()
+	}
+
+	li, err := instances.GetLibrariesIndex(req.GetInstance())
 	if err != nil {
 		return nil, err
 	}
 
 	logrus.Info("Preparing download")
 
-	lib, err := findLibraryIndexRelease(lm.Index, req)
+	version, err := commands.ParseVersion(req.GetVersion())
+	if err != nil {
+		return nil, err
+	}
+
+	lib, err := li.FindRelease(req.GetName(), version)
 	if err != nil {
 		return nil, err
 	}
 
-	if err := downloadLibrary(lm, lib, downloadCB, func(*rpc.TaskProgress) {}, "download"); err != nil {
+	if err := downloadLibrary(downloadsDir, lib, downloadCB, func(*rpc.TaskProgress) {}, "download"); err != nil {
 		return nil, err
 	}
 
 	return &rpc.LibraryDownloadResponse{}, nil
 }
 
-func downloadLibrary(lm *librariesmanager.LibrariesManager, libRelease *librariesindex.Release,
+func downloadLibrary(downloadsDir *paths.Path, libRelease *librariesindex.Release,
 	downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string) error {
 
 	taskCB(&rpc.TaskProgress{Name: tr("Downloading %s", libRelease)})
 	config, err := httpclient.GetDownloaderConfig()
 	if err != nil {
 		return &cmderrors.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
 	}
-	if err := libRelease.Resource.Download(lm.DownloadsDir, config, libRelease.String(), downloadCB, queryParameter); err != nil {
+	if err := libRelease.Resource.Download(downloadsDir, config, libRelease.String(), downloadCB, queryParameter); err != nil {
 		return &cmderrors.FailedDownloadError{Message: tr("Can't download library"), Cause: err}
 	}
 	taskCB(&rpc.TaskProgress{Completed: true})