gRPC: if an Upload request is canceled, immediately terminate the upload tool process. (#2726)

* Added integration test

* Fixed process termination when upload call is canceled

Author: cmaglie

Diff for: commands/service_upload.go

@@ -573,18 +573,18 @@ func (s *arduinoCoreServerImpl) runProgramAction(ctx context.Context, pme *packa
 	// Run recipes for upload
 	toolEnv := pme.GetEnvVarsForSpawnedProcess()
 	if burnBootloader {
-		if err := runTool("erase.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
+		if err := runTool(uploadCtx, "erase.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
 			return nil, &cmderrors.FailedUploadError{Message: i18n.Tr("Failed chip erase"), Cause: err}
 		}
-		if err := runTool("bootloader.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
+		if err := runTool(uploadCtx, "bootloader.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
 			return nil, &cmderrors.FailedUploadError{Message: i18n.Tr("Failed to burn bootloader"), Cause: err}
 		}
 	} else if programmer != nil {
-		if err := runTool("program.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
+		if err := runTool(uploadCtx, "program.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
 			return nil, &cmderrors.FailedUploadError{Message: i18n.Tr("Failed programming"), Cause: err}
 		}
 	} else {
-		if err := runTool("upload.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
+		if err := runTool(uploadCtx, "upload.pattern", uploadProperties, outStream, errStream, verbose, dryRun, toolEnv); err != nil {
 			return nil, &cmderrors.FailedUploadError{Message: i18n.Tr("Failed uploading"), Cause: err}
 		}
 	}
@@ -702,7 +702,12 @@ func detectUploadPort(
 	}
 }
 
-func runTool(recipeID string, props *properties.Map, outStream, errStream io.Writer, verbose bool, dryRun bool, toolEnv []string) error {
+func runTool(ctx context.Context, recipeID string, props *properties.Map, outStream, errStream io.Writer, verbose bool, dryRun bool, toolEnv []string) error {
+	// if ctx is already canceled just exit
+	if err := ctx.Err(); err != nil {
+		return err
+	}
+
 	recipe, ok := props.GetOk(recipeID)
 	if !ok {
 		return errors.New(i18n.Tr("recipe not found '%s'", recipeID))
@@ -739,6 +744,17 @@ func runTool(recipeID string, props *properties.Map, outStream, errStream io.Wri
 		return errors.New(i18n.Tr("cannot execute upload tool: %s", err))
 	}
 
+	// If the ctx is canceled, kill the running command
+	completed := make(chan struct{})
+	defer close(completed)
+	go func() {
+		select {
+		case <-ctx.Done():
+			_ = cmd.Kill()
+		case <-completed:
+		}
+	}()
+
 	if err := cmd.Wait(); err != nil {
 		return errors.New(i18n.Tr("uploading error: %s", err))
 	}

Diff for: internal/integrationtest/arduino-cli.go

@@ -286,6 +286,42 @@ func (cli *ArduinoCLI) InstallMockedSerialMonitor(t *testing.T) {
 	}
 }
 
+// InstallMockedAvrdude will replace the already installed avrdude with a mocked one.
+func (cli *ArduinoCLI) InstallMockedAvrdude(t *testing.T) {
+	fmt.Println(color.BlueString("<<< Install mocked avrdude"))
+
+	// Build mocked serial-discovery
+	mockDir := FindRepositoryRootPath(t).Join("internal", "mock_avrdude")
+	gobuild, err := paths.NewProcess(nil, "go", "build")
+	require.NoError(t, err)
+	gobuild.SetDirFromPath(mockDir)
+	require.NoError(t, gobuild.Run(), "Building mocked avrdude")
+	ext := ""
+	if runtime.GOOS == "windows" {
+		ext = ".exe"
+	}
+	mockBin := mockDir.Join("mock_avrdude" + ext)
+	require.True(t, mockBin.Exist())
+	fmt.Println(color.HiBlackString("    Build of mocked avrdude succeeded."))
+
+	// Install it replacing the current avrdudes
+	dataDir := cli.DataDir()
+	require.NotNil(t, dataDir, "data dir missing")
+
+	avrdudes, err := dataDir.Join("packages", "arduino", "tools", "avrdude").ReadDirRecursiveFiltered(
+		nil, paths.AndFilter(
+			paths.FilterNames("avrdude"+ext),
+			paths.FilterOutDirectories(),
+		),
+	)
+	require.NoError(t, err, "scanning data dir for avrdude(s)")
+	require.NotEmpty(t, avrdudes, "no avrdude(s) found in data dir")
+	for _, avrdude := range avrdudes {
+		require.NoError(t, mockBin.CopyTo(avrdude), "installing mocked avrdude to %s", avrdude)
+		fmt.Println(color.HiBlackString("    Mocked avrdude installed in " + avrdude.String()))
+	}
+}
+
 // RunWithCustomEnv executes the given arduino-cli command with the given custom env and returns the output.
 func (cli *ArduinoCLI) RunWithCustomEnv(env map[string]string, args ...string) ([]byte, []byte, error) {
 	var stdoutBuf, stderrBuf bytes.Buffer
@@ -642,3 +678,19 @@ func (inst *ArduinoCLIInstance) Monitor(ctx context.Context, port *commands.Port
 	})
 	return monitorClient, err
 }
+
+// Upload calls the "Upload" gRPC method.
+func (inst *ArduinoCLIInstance) Upload(ctx context.Context, fqbn, sketchPath, port, protocol string) (commands.ArduinoCoreService_UploadClient, error) {
+	uploadCl, err := inst.cli.daemonClient.Upload(ctx, &commands.UploadRequest{
+		Instance:   inst.instance,
+		Fqbn:       fqbn,
+		SketchPath: sketchPath,
+		Verbose:    true,
+		Port: &commands.Port{
+			Address:  port,
+			Protocol: protocol,
+		},
+	})
+	logCallf(">>> Upload(%v %v port/protocol=%s/%s)\n", fqbn, sketchPath, port, protocol)
+	return uploadCl, err
+}

Diff for: internal/integrationtest/daemon/upload_test.go

@@ -0,0 +1,120 @@
+// This file is part of arduino-cli.
+//
+// Copyright 2024 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to
+// modify or otherwise use the software for commercial activities involving the
+// Arduino software without disclosing the source code of your own applications.
+// To purchase a commercial license, send an email to [email protected].
+
+package daemon_test
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"io"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/arduino/arduino-cli/internal/integrationtest"
+	"github.com/arduino/arduino-cli/rpc/cc/arduino/cli/commands/v1"
+	"github.com/arduino/go-paths-helper"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUploadCancelation(t *testing.T) {
+	env, cli := integrationtest.CreateEnvForDaemon(t)
+	defer env.CleanUp()
+
+	grpcInst := cli.Create()
+	require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+		fmt.Printf("INIT> %v\n", ir.GetMessage())
+	}))
+
+	plInst, err := grpcInst.PlatformInstall(context.Background(), "arduino", "avr", "1.8.6", true)
+	require.NoError(t, err)
+	for {
+		msg, err := plInst.Recv()
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		require.NoError(t, err)
+		fmt.Printf("INSTALL> %v\n", msg)
+	}
+
+	// Mock avrdude
+	cli.InstallMockedAvrdude(t)
+
+	// Re-init instance to update changes
+	require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) {
+		fmt.Printf("INIT> %v\n", ir.GetMessage())
+	}))
+
+	// Build sketch for upload
+	sk := paths.New("testdata", "bare_minimum")
+	compile, err := grpcInst.Compile(context.Background(), "arduino:avr:uno", sk.String(), "")
+	require.NoError(t, err)
+	for {
+		msg, err := compile.Recv()
+		if errors.Is(err, io.EOF) {
+			break
+		}
+		if err != nil {
+			fmt.Println("COMPILE ERROR>", err)
+			require.FailNow(t, "Expected successful compile", "compilation failed")
+			break
+		}
+		if msg.GetOutStream() != nil {
+			fmt.Printf("COMPILE OUT> %v\n", string(msg.GetOutStream()))
+		}
+		if msg.GetErrStream() != nil {
+			fmt.Printf("COMPILE ERR> %v\n", string(msg.GetErrStream()))
+		}
+	}
+
+	// Try upload and interrupt the call after 1 sec
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	upload, err := grpcInst.Upload(ctx, "arduino:avr:uno", sk.String(), "/dev/ttyACM0", "serial")
+	require.NoError(t, err)
+	checkFile := ""
+	for {
+		msg, err := upload.Recv()
+		if errors.Is(err, io.EOF) {
+			require.FailNow(t, "Expected interrupted upload", "upload succeeded")
+			break
+		}
+		if err != nil {
+			fmt.Println("UPLOAD ERROR>", err)
+			break
+		}
+		if out := string(msg.GetOutStream()); out != "" {
+			fmt.Printf("UPLOAD OUT> %v\n", out)
+			if strings.HasPrefix(out, "CHECKFILE: ") {
+				checkFile = strings.TrimSpace(out[11:])
+			}
+		}
+		if msg.GetErrStream() != nil {
+			fmt.Printf("UPLOAD ERR> %v\n", string(msg.GetErrStream()))
+		}
+	}
+	cancel()
+
+	// Wait 5 seconds.
+	// If the mocked avrdude is not killed it will create a checkfile and it will remove it after 5 seconds.
+	time.Sleep(5 * time.Second)
+
+	// Test if the checkfile is still there (if the file is there it means that mocked avrdude
+	// has been correctly killed).
+	require.NotEmpty(t, checkFile)
+	require.FileExists(t, checkFile)
+	require.NoError(t, os.Remove(checkFile))
+}

Diff for: internal/mock_avrdude/.gitignore

@@ -0,0 +1 @@
+mock_avrdude

Diff for: internal/mock_avrdude/main.go

@@ -0,0 +1,46 @@
+//
+// This file is part arduino-cli.
+//
+// Copyright 2023 ARDUINO SA (http://www.arduino.cc/)
+//
+// This software is released under the GNU General Public License version 3,
+// which covers the main part of arduino-cli.
+// The terms of this license can be found at:
+// https://www.gnu.org/licenses/gpl-3.0.en.html
+//
+// You can be released from the requirements of the above licenses by purchasing
+// a commercial license. Buying such a license is mandatory if you want to modify or
+// otherwise use the software for commercial activities involving the Arduino
+// software without disclosing the source code of your own applications. To purchase
+// a commercial license, send an email to [email protected].
+//
+
+package main
+
+import (
+	"fmt"
+	"os"
+	"time"
+
+	"github.com/arduino/go-paths-helper"
+)
+
+func main() {
+	tmp, err := paths.MkTempFile(nil, "test")
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+	tmp.Close()
+	tmpPath := paths.New(tmp.Name())
+
+	fmt.Println("CHECKFILE:", tmpPath)
+
+	// Just sit here for 5 seconds
+	time.Sleep(5 * time.Second)
+
+	// Remove the check file at the end
+	tmpPath.Remove()
+
+	fmt.Println("COMPLETED")
+}