From 958fbf73300cb6ab8d0f8644c681e5a29a1801e0 Mon Sep 17 00:00:00 2001
From: Michael Whittaker <mwhittaker@users.noreply.github.com>
Date: Mon, 21 Aug 2023 10:34:03 -0700
Subject: [PATCH] Fixed bugs in RemoteWeavelet UpdateComponents. (#530)

This PR fixes two small bugs in UpdateComponents.

1. UpdateComponents now returns an error if the provided components
   don't exist. Previously, the error was logged but not returned.
2. Previously, if a component failed to construct, the construction of
   all subsequent components was short circuited. Now, we try to
   construct all components, even if some fail to construct.
---
 godeps.txt                                   |  1 -
 internal/testdeployer/remoteweavelet_test.go |  8 +---
 internal/weaver/remoteweavelet.go            | 42 +++++++++++---------
 3 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/godeps.txt b/godeps.txt
index 90c87ad20..32b92b0da 100644
--- a/godeps.txt
+++ b/godeps.txt
@@ -571,7 +571,6 @@ github.com/ServiceWeaver/weaver/internal/weaver
     os/signal
     path/filepath
     reflect
-    slices
     sort
     strings
     sync
diff --git a/internal/testdeployer/remoteweavelet_test.go b/internal/testdeployer/remoteweavelet_test.go
index cd79415c3..5ed6f053b 100644
--- a/internal/testdeployer/remoteweavelet_test.go
+++ b/internal/testdeployer/remoteweavelet_test.go
@@ -483,12 +483,8 @@ func TestUpdateMissingComponents(t *testing.T) {
 
 	// Update the weavelet with components that don't exist.
 	components := &protos.UpdateComponentsRequest{Components: []string{"foo", "bar"}}
-	if _, err := d.wlet.UpdateComponents(components); err != nil {
-		// TODO(mwhittaker): Right now, UpdateComponents always returns nil and
-		// updates components in the background. This is to avoid inducing
-		// deadlock in deployers. We should probably return an error here when
-		// possible.
-		t.Fatal(err)
+	if _, err := d.wlet.UpdateComponents(components); err == nil {
+		t.Fatal("unexpected success")
 	}
 
 	testComponents(d)
diff --git a/internal/weaver/remoteweavelet.go b/internal/weaver/remoteweavelet.go
index 8023a33df..37074cb23 100644
--- a/internal/weaver/remoteweavelet.go
+++ b/internal/weaver/remoteweavelet.go
@@ -24,7 +24,6 @@ import (
 	"net"
 	"os"
 	"reflect"
-	"slices"
 	"strings"
 	"sync"
 	"time"
@@ -392,6 +391,24 @@ func (w *RemoteWeavelet) GetLoad(*protos.GetLoadRequest) (*protos.GetLoadReply,
 
 // UpdateComponents implements the conn.WeaverHandler interface.
 func (w *RemoteWeavelet) UpdateComponents(req *protos.UpdateComponentsRequest) (*protos.UpdateComponentsReply, error) {
+	var errs []error
+	var components []*component
+	var shortened []string
+	for _, component := range req.Components {
+		short := fmt.Sprintf("%q", logging.ShortenComponent(component))
+		shortened = append(shortened, short)
+		c, err := w.getComponent(component)
+		if err != nil {
+			w.syslogger.Error(fmt.Sprintf("Failed to update component %s", short), "err", err)
+			errs = append(errs, err)
+			continue
+		}
+		components = append(components, c)
+	}
+	if len(components) == 0 {
+		return &protos.UpdateComponentsReply{}, errors.Join(errs...)
+	}
+
 	// Create components in a separate goroutine. A component's Init function
 	// may be slow or block. It may also trigger pipe communication. We want to
 	// avoid blocking and pipe communication in this handler as it could cause
@@ -402,29 +419,18 @@ func (w *RemoteWeavelet) UpdateComponents(req *protos.UpdateComponentsRequest) (
 	//
 	// TODO(mwhittaker): Document that handlers shouldn't retain access to the
 	// arguments passed to them.
-	components := slices.Clone(req.Components)
 	go func() {
-		shortened := make([]string, len(components))
 		for i, c := range components {
-			shortened[i] = fmt.Sprintf("%q", logging.ShortenComponent(c))
-		}
-		w.syslogger.Debug(fmt.Sprintf("Updating components %v", shortened))
-		for _, component := range components {
-			c, err := w.getComponent(component)
-			if err != nil {
-				// TODO(mwhittaker): Propagate errors.
-				w.syslogger.Debug(fmt.Sprintf("Failed to update components %v", shortened), "err", err)
-				return
-			}
-			if _, err = w.GetImpl(c.reg.Impl); err != nil {
+			w.syslogger.Debug(fmt.Sprintf("Updating component %s", shortened[i]))
+			if _, err := w.GetImpl(c.reg.Impl); err != nil {
 				// TODO(mwhittaker): Propagate errors.
-				w.syslogger.Debug(fmt.Sprintf("Failed to update components %v", shortened), "err", err)
-				return
+				w.syslogger.Error(fmt.Sprintf("Failed to update component %v", shortened[i]), "err", err)
+				continue
 			}
+			w.syslogger.Debug(fmt.Sprintf("Updated component %s", shortened[i]))
 		}
-		w.syslogger.Debug(fmt.Sprintf("Updated components %v", shortened))
 	}()
-	return &protos.UpdateComponentsReply{}, nil
+	return &protos.UpdateComponentsReply{}, errors.Join(errs...)
 }
 
 // UpdateRoutingInfo implements the conn.WeaverHandler interface.