veyron/lib/exec: Allow SetReady to be called multiple times.
We cache the error generated by the first call and return it directly
on subsequent calls. This allows us to check the SetReady error
in the runtime initialization code even though the modules framework
sets ready itself when you run in a module.
Change-Id: I279e37ff88ddc698e274241d2037c133a3916fce
diff --git a/lib/exec/child.go b/lib/exec/child.go
index 09b53fe..688a74b 100644
--- a/lib/exec/child.go
+++ b/lib/exec/child.go
@@ -26,6 +26,15 @@
// veyron framework to notify the parent that the child process has
// successfully started.
statusPipe *os.File
+
+ // statusMu protexts sentStatus and statusErr and prevents us from trying to
+ // send multiple status updates to the parent.
+ statusMu sync.Mutex
+ // sentStatus records the status that was already sent to the parent.
+ sentStatus string
+ // statusErr records the error we received, if any, when sentStatus
+ // was sent.
+ statusErr error
}
var (
@@ -57,35 +66,43 @@
return childHandle, childHandleErr
}
-func (c *ChildHandle) writeStatus(status string) error {
- toWrite := make([]byte, 0, len(status))
- var buf [utf8.UTFMax]byte
- // This replaces any invalid utf-8 bytes in the status string with the
- // Unicode replacement character. This ensures that we only send valid
- // utf-8 (followed by the eofChar).
- for _, r := range status {
- n := utf8.EncodeRune(buf[:], r)
- toWrite = append(toWrite, buf[:n]...)
+func (c *ChildHandle) writeStatus(status, detail string) error {
+ c.statusMu.Lock()
+ defer c.statusMu.Unlock()
+
+ if c.sentStatus == "" {
+ c.sentStatus = status
+ status = status + detail
+ toWrite := make([]byte, 0, len(status))
+ var buf [utf8.UTFMax]byte
+ // This replaces any invalid utf-8 bytes in the status string with the
+ // Unicode replacement character. This ensures that we only send valid
+ // utf-8 (followed by the eofChar).
+ for _, r := range status {
+ n := utf8.EncodeRune(buf[:], r)
+ toWrite = append(toWrite, buf[:n]...)
+ }
+ toWrite = append(toWrite, eofChar)
+ _, c.statusErr = c.statusPipe.Write(toWrite)
+ c.statusPipe.Close()
+ } else if c.sentStatus != status {
+ return errors.New("A different status: " + c.sentStatus + " has already been sent.")
}
- toWrite = append(toWrite, eofChar)
- _, err := c.statusPipe.Write(toWrite)
- c.statusPipe.Close()
- return err
+ return c.statusErr
}
-// TODO(caprita): There's nothing preventing SetReady and SetFailed from being
-// called multiple times (e.g. from different instances of the runtime
-// intializing themselves). This results in errors for all but the first
-// invocation. Should we instead run these with sync.Once?
-
// SetReady writes a 'ready' status to its parent.
+// Only one of SetReady or SetFailed can be called, attempting to send
+// both will fail. In addition the status is only sent once to the parent
+// subsequent calls will return immediately with the same error that was
+// returned on the first call (possibly nil).
func (c *ChildHandle) SetReady() error {
- return c.writeStatus(readyStatus + strconv.Itoa(os.Getpid()))
+ return c.writeStatus(readyStatus, strconv.Itoa(os.Getpid()))
}
// SetFailed writes a 'failed' status to its parent.
func (c *ChildHandle) SetFailed(oerr error) error {
- return c.writeStatus(failedStatus + oerr.Error())
+ return c.writeStatus(failedStatus, oerr.Error())
}
// NewExtraFile creates a new file handle for the i-th file descriptor after
diff --git a/lib/exec/exec_test.go b/lib/exec/exec_test.go
index f2ca9db..44f92a5 100644
--- a/lib/exec/exec_test.go
+++ b/lib/exec/exec_test.go
@@ -536,17 +536,28 @@
log.Fatal(err)
}
verifyNoExecVariable()
- if err := ch.SetFailed(fmt.Errorf("%s", strings.Join(args, " "))); err != nil {
-
+ err = ch.SetFailed(fmt.Errorf("%s", strings.Join(args, " ")))
+ if err != nil {
fmt.Fprintf(os.Stderr, "%s\n", err)
}
+ // It's fine to call SetFailed multiple times.
+ if err2 := ch.SetFailed(fmt.Errorf("dummy")); err != err2 {
+ fmt.Fprintf(os.Stderr, "Received new error got: %v, want %v\n", err2, err)
+ }
case "testReady":
ch, err := vexec.GetChildHandle()
if err != nil {
log.Fatal(err)
}
verifyNoExecVariable()
- ch.SetReady()
+ err = ch.SetReady()
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "%s\n", err)
+ }
+ // It's fine to call SetReady multiple times.
+ if err2 := ch.SetReady(); err != err2 {
+ fmt.Fprintf(os.Stderr, "Received new error got: %v, want %v\n", err2, err)
+ }
fmt.Fprintf(os.Stderr, ".")
case "testReadySlow":
ch, err := vexec.GetChildHandle()
diff --git a/runtimes/google/rt/mgmt.go b/runtimes/google/rt/mgmt.go
index ccaad62..56240d6 100644
--- a/runtimes/google/rt/mgmt.go
+++ b/runtimes/google/rt/mgmt.go
@@ -59,14 +59,7 @@
return err
}
- // Note(mattr): that we ignore the error here.
- // As far as I can tell this is because the modules framework actually calls
- // SetReady(), so we would then call it here a second time and get
- // an error.
- // TODO(mattr): We should change the modules framework so we can properly check
- // errors here.
- handle.SetReady()
- return nil
+ return handle.SetReady()
}
func getListenSpec(handle *exec.ChildHandle) (*ipc.ListenSpec, error) {
