Skip to content

Commit

Permalink
Fixed bugs in RemoteWeavelet UpdateComponents. (#530)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mwhittaker authored Aug 21, 2023
1 parent 1a00443 commit 958fbf7
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 25 deletions.
1 change: 0 additions & 1 deletion godeps.txt
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,6 @@ github.com/ServiceWeaver/weaver/internal/weaver
os/signal
path/filepath
reflect
slices
sort
strings
sync
Expand Down
8 changes: 2 additions & 6 deletions internal/testdeployer/remoteweavelet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
42 changes: 24 additions & 18 deletions internal/weaver/remoteweavelet.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"net"
"os"
"reflect"
"slices"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down

0 comments on commit 958fbf7

Please sign in to comment.