Re: Strange lock-ups with simple E program [patch]

>From 0323f169ab62aee6f41e5167ea77ed4b4d84d548 Mon Sep 17 00:00:00 2001
From: Thomas Leonard <tal-v5nx5w6akNyLE8xUarVfuPLx9OUvmyODWmv/[email protected]>
Date: Tue, 24 Nov 2009 10:21:48 +0000
Subject: [PATCH] Fixed race condition in the boot-comm system

When queuing an event in another vat on the same machine, we tried to construct
a BootRefHandler for the promise as the promise was being resolved in another
thread. This could lead to various race conditions, including errors such as
"OldFarRef may only be smashed", "ViciousCycleException" and deadlock).

Now, we create the BootRefHandler for the promise of the result before adding
it to the queue.

This might also mean that a number of "this method must be thread-safe"
requirements can be dropped.

Patch provided by the University of Southampton IT Innovation Centre.
---
 .../org/erights/e/elib/vat/BootRefHandler.java     |   75 +++++++++++++-------
 src/jsrc/org/erights/e/elib/vat/Vat.java           |   32 +++++++--
 2 files changed, 77 insertions(+), 30 deletions(-)

diff --git a/src/jsrc/org/erights/e/elib/vat/BootRefHandler.java b/src/jsrc/org/erights/e/elib/vat/BootRefHandler.java
index e6d0818..4144910 100644
--- a/src/jsrc/org/erights/e/elib/vat/BootRefHandler.java
+++ b/src/jsrc/org/erights/e/elib/vat/BootRefHandler.java
@@ -9,6 +9,7 @@ import org.erights.e.elib.ref.DelayedRedirector;
 import org.erights.e.elib.ref.EProxyHandler;
 import org.erights.e.elib.ref.EProxyResolver;
 import org.erights.e.elib.ref.Ref;
+import org.erights.e.elib.ref.Resolver;
 import org.erights.e.elib.sealing.Brand;
 import org.erights.e.elib.sealing.SealedBox;
 import org.erights.e.elib.sealing.Sealer;
@@ -119,8 +120,8 @@ class BootRefHandler implements EProxyHandler {
      * except that it's thread-safe.
      * <p/>
      * <tt>getOptBootRefHandler/1</tt> must be thread safe, in order for {@link
-     * org.erights.e.elib.vat.BootRefHandler#packageArg(Object,Vat,Vat,Vat)
-     * BootRefHandler.packageArg/4} to be thread safe: Callers of this should
+     * org.erights.e.elib.vat.BootRefHandler#packageArg(Object,Vat,Vat)
+     * BootRefHandler.packageArg/3} to be thread safe: Callers of this should
      * keep in mind that ref may be shortened after the handler is gotten but
      * before these callers use it. If they access only final fields of the
      * handler in a thread safe way, then everything should be fine.
@@ -153,15 +154,16 @@ class BootRefHandler implements EProxyHandler {
      * to the eventual reference in the src vat, where the argument is a
      * boot-ref on a {@link DelayedRedirector} on the returned boot-ref. <li>In
      * all other cases, an exception is thrown. </ul>
+     * <p/>
+     * If <tt>arg</tt> is not in <tt>src</tt> then you must ensure that it
+     * won't change during this method call (this is only used in the
+     * recursive case).
      *
      * @param arg        The reference to be packaged.
-     * @param src        The vat that 'arg' is valid within.
+     * @param src        The vat that 'arg' is valid within
      * @param dest       The vat the return result needs to be valid within.
-     * @param currentVat The vat within which we're currently executing, which
-     *                   may be src, dest, or a third introducing vat (Alice).
-     *                   This is used to resolve race conditions.
      */
-    private Object packageArg(Object arg, Vat src, Vat dest, Vat currentVat) {
+    private Object packageArg(Object arg, Vat src, Vat dest) {
         arg = Ref.resolution(arg);
         String state = Ref.state(arg);
         arg = Ref.resolution(arg);
@@ -187,7 +189,7 @@ class BootRefHandler implements EProxyHandler {
                 Object[] result = new Object[argList.size()];
                 for (int i = 0, len = result.length; i < len; i++) {
                     result[i] =
-                      packageArg(argList.get(i), src, dest, currentVat);
+                      packageArg(argList.get(i), src, dest);
                 }
                 return ConstList.fromArray(result);
             }
@@ -197,9 +199,9 @@ class BootRefHandler implements EProxyHandler {
                 //XXX Perhaps we should abstract out arrays as a separate case?
                 ConstMap argMap = (ConstMap)arg;
                 ConstList keys = ConstList.fromArray(argMap.getKeys());
-                keys = (ConstList)packageArg(keys, src, dest, currentVat);
+                keys = (ConstList)packageArg(keys, src, dest);
                 ConstList vals = ConstList.fromArray(argMap.getValues());
-                vals = (ConstList)packageArg(vals, src, dest, currentVat);
+                vals = (ConstList)packageArg(vals, src, dest);
                 try {
                     return ConstMap.fromColumns(keys, vals);
                 } catch (ArityMismatchException e) {
@@ -229,23 +231,22 @@ class BootRefHandler implements EProxyHandler {
                 T.require(src != optHandler.myTargetsVat,
                           "Unshortened boot-ref: ",
                           arg);
+                //XXX This doesn't look safe; what if optHandler.myTarget changes
+                //while we're packaging it? We could just fall-through to the code
+                //below, but that seems to break one of the test cases.
                 return packageArg(optHandler.myTarget,
                                   optHandler.myTargetsVat,
-                                  dest,
-                                  currentVat);
+                                  dest);
             }
 
-            //arg is EVENTUAL (or was at the time of the test), and is not
-            //handled by a BootRefHandler, so we treat it as a promise in the
-            //src vat.
-
             BootRefHandler handler = new BootRefHandler(src, arg);
             DelayedRedirector rdr = new DelayedRedirector(handler.myResolver);
-            //handler and rdr are in the dest vat
-            Object[] args = {packageArg(rdr, dest, src, currentVat)};
+            //handler and rdr will be in the dest vat
+            Object[] args = {packageArg(rdr, dest, src)};
             src.qSendAllOnly(arg, false, "__whenMoreResolved", args);
             //Is it ok to ignore the E.sendOnly return result here?
             return handler.myResolver.getProxy();
+
         }
     }
 
@@ -258,7 +259,7 @@ class BootRefHandler implements EProxyHandler {
         Object[] result = new Object[args.length];
         for (int i = 0, len = args.length; i < len; i++) {
             result[i] =
-              packageArg(args[i], currentVat, myTargetsVat, currentVat);
+              packageArg(args[i], currentVat, myTargetsVat);
         }
         return result;
     }
@@ -281,13 +282,37 @@ class BootRefHandler implements EProxyHandler {
      */
     public Ref handleSendAll(String verb, Object[] args) {
         myFreshFlag = false;
-        Ref promise =
-          myTargetsVat.qSendAll(myTarget, false, verb, packageArgs(args));
+
+        Object[] promisePair = Ref.promise();
+        Ref promise = (Ref)promisePair[0];  // In myTargetsVat
+
         Vat currentVat = Vat.getCurrentVat();
-        return Ref.toRef(packageArg(promise,
-                                    myTargetsVat,
-                                    currentVat,
-                                    currentVat));
+
+        Ref remotePromise;                  // In currentVat
+
+        if (myTargetsVat == currentVat) {
+            remotePromise = promise;
+        } else {
+            BootRefHandler handler = new BootRefHandler(myTargetsVat, promise);
+            Ref proxy = handler.myResolver.getProxy();
+
+            DelayedRedirector rdr = new DelayedRedirector(handler.myResolver);
+            //handler and rdr are in the dest (current) vat
+            Object[] moreArgs = {packageArg(rdr, currentVat, myTargetsVat)};
+            myTargetsVat.qSendAllOnly(promise, false, "__whenMoreResolved", moreArgs);
+            //Is it ok to ignore the E.sendOnly return result here?
+
+            remotePromise = proxy;
+        }
+
+        Throwable optProblem =
+          myTargetsVat.qSendAll(myTarget, false, verb, packageArgs(args), (Resolver)promisePair[1]);
+
+        if (null == optProblem) {
+            return remotePromise;
+        } else {
+            return Ref.broken(optProblem);
+        }
     }
 
     /**
diff --git a/src/jsrc/org/erights/e/elib/vat/Vat.java b/src/jsrc/org/erights/e/elib/vat/Vat.java
index 57a2d1b..539ab49 100644
--- a/src/jsrc/org/erights/e/elib/vat/Vat.java
+++ b/src/jsrc/org/erights/e/elib/vat/Vat.java
@@ -369,20 +369,21 @@ public class Vat {
      * that this event might never be delivered, then the reference eventually
      * becomes broken with a complaint explaining why.
      * <p/>
-     * May be called from any thead.
+     * Must only be called from the vat's own thread, since otherwise the
+     * returned promise may change as you access it (and, in particular, it
+     * may briefly turn into a ViciousCycleException).
      * <p/>
      * XXX to be made non-public. Uses outside this package should use {@link
      * BootRefHandler boot-refs} instead.
      */
-    public Ref qSendAll(Object rec,
+    Ref qSendAll(Object rec,
                         boolean nowFlag,
                         String verb,
                         Object[] args) {
         Object[] promise = Ref.promise();
         Resolver resolver = (Resolver)promise[1];
-        PendingDelivery pe =
-          new PendingDelivery(this, rec, resolver, nowFlag, verb, args);
-        Throwable optProblem = getRunner().enqueue(pe);
+
+        Throwable optProblem = qSendAll(rec, nowFlag, verb, args, resolver);
         if (null == optProblem) {
             return (Ref)promise[0];
         } else {
@@ -391,6 +392,27 @@ public class Vat {
     }
 
     /**
+     * Enqueues a 'rec <- verb(args...)'.
+     * <p/>
+     * If this vat is shut down, returns a Throwable explaining the problem.
+     * Otherwise, if it becomes known that this event might never be delivered,
+     * then the reference eventually becomes broken with a complaint explaining
+     * why.
+     * <p/>
+     * The only outside user of this call is {@link BootRefHandler}; everyone else
+     * should go via that.
+     */
+    public Throwable qSendAll(Object rec,
+                        boolean nowFlag,
+                        String verb,
+                        Object[] args,
+                        Resolver resolver) {
+        PendingDelivery pe =
+          new PendingDelivery(this, rec, resolver, nowFlag, verb, args);
+        return getRunner().enqueue(pe);
+    }
+
+    /**
      * Schedules a Runnable to execute in a Runner (in the Runner's thread as a
      * separate turn), while also effectively executing as a synchronous call
      * within the requestors's {@link Runner#getOptCurrentRunner() external
-- 
1.6.3.3