Object safepoint operations need to be in finally blocks consistently

This is a correctness precaution. All subsequent register/unregister operations following an initial unregister/register need to be in a finally clause if the operations in-between could cause any kind of exception, which most can. For consistency, it should be used really everywhere. There are only two points where it might be acceptable, but should be still fixed: init of object system, and in the executeApplication method. Signed-off-by: Stefan Marr <git@stefan-marr.de>
smarr · Apr 16, 2020 · dd07d87 · dd07d87
1 parent 352e0e6
commit dd07d87
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 33 deletions.
diff --git a/src/som/interpreter/actors/Actor.java b/src/som/interpreter/actors/Actor.java
@@ -259,6 +259,19 @@ void doRun() {
       ObjectTransitionSafepoint.INSTANCE.register();
 
       ActorProcessingThread t = (ActorProcessingThread) Thread.currentThread();
+      try {
+        doRunWithObjectSafepoints(t);
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.unregister();
+
+        if (VmSettings.ACTOR_TRACING || VmSettings.KOMPOS_TRACING) {
+          t.swapTracingBufferIfRequestedUnsync();
+        }
+        t.currentlyExecutingActor = null;
+      }
+    }
+
+    public void doRunWithObjectSafepoints(final ActorProcessingThread t) {
       WebDebugger dbg = null;
       if (VmSettings.TRUFFLE_DEBUGGER_ENABLED) {
         dbg = vm.getWebDebugger();
@@ -273,18 +286,9 @@ void doRun() {
         KomposTrace.currentActivity(actor);
       }
 
-      try {
-        while (getCurrentMessagesOrCompleteExecution()) {
-          processCurrentMessages(t, dbg);
-        }
-      } finally {
-        ObjectTransitionSafepoint.INSTANCE.unregister();
-      }
-
-      if (VmSettings.ACTOR_TRACING || VmSettings.KOMPOS_TRACING) {
-        t.swapTracingBufferIfRequestedUnsync();
+      while (getCurrentMessagesOrCompleteExecution()) {
+        processCurrentMessages(t, dbg);
       }
-      t.currentlyExecutingActor = null;
     }
 
     protected void processCurrentMessages(final ActorProcessingThread currentThread,

diff --git a/src/som/interpreter/processes/SChannel.java b/src/som/interpreter/processes/SChannel.java
@@ -86,11 +86,11 @@ public SChannelInput(final SynchronousQueue<Object> cell,
     public Object read(final RecordTwoEvent traceRead) throws InterruptedException {
       ObjectTransitionSafepoint.INSTANCE.unregister();
 
-      if (VmSettings.REPLAY) {
-        ReplayData.replayDelayNumberedEvent(this, channel.getId());
-      }
-
       try {
+        if (VmSettings.REPLAY) {
+          ReplayData.replayDelayNumberedEvent(this, channel.getId());
+        }
+
         synchronized (this) {
           if (VmSettings.ACTOR_TRACING) {
             traceRead.record(channel.getId(), numReads);
@@ -161,11 +161,11 @@ public void write(final Object value, final RecordTwoEvent traceWrite)
         throws InterruptedException {
       ObjectTransitionSafepoint.INSTANCE.unregister();
 
-      if (VmSettings.REPLAY) {
-        ReplayData.replayDelayNumberedEvent(this, channel.getId());
-      }
-
       try {
+        if (VmSettings.REPLAY) {
+          ReplayData.replayDelayNumberedEvent(this, channel.getId());
+        }
+
         synchronized (this) {
           if (VmSettings.ACTOR_TRACING) {
             traceWrite.record(channel.getId(), numWrites);

diff --git a/src/som/primitives/threading/ConditionPrimitives.java b/src/som/primitives/threading/ConditionPrimitives.java
@@ -47,9 +47,9 @@ public final Condition doCondition(final Condition cond) {
         cond.await();
       } catch (InterruptedException e) {
         /* doesn't tell us a lot at the moment, so it is ignored */
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.register();
       }
-
-      ObjectTransitionSafepoint.INSTANCE.register();
       return cond;
     }
   }

diff --git a/src/som/primitives/threading/DelayPrimitives.java b/src/som/primitives/threading/DelayPrimitives.java
@@ -23,8 +23,9 @@ public final SObjectWithoutFields doLong(final long milliseconds) {
         Thread.sleep(milliseconds);
       } catch (InterruptedException e) {
         /* Not relevant for the moment */
+      } finally {
+        ObjectTransitionSafepoint.INSTANCE.register();
       }
-      ObjectTransitionSafepoint.INSTANCE.register();
       return Nil.nilObject;
     }
   }

diff --git a/src/tools/concurrency/TracingActors.java b/src/tools/concurrency/TracingActors.java
@@ -375,22 +375,28 @@ private static void removeFirstExpectedMessage(final ReplayActor a) {
     private MessageRecord getExpected() {
       if (expectedMessages.isEmpty()) {
         ObjectTransitionSafepoint.INSTANCE.unregister();
-        while (traceParser.getMoreEventsForEntity(getId())
-            && expectedMessages.isEmpty()) {
-          // NOOP
+        try {
+          while (traceParser.getMoreEventsForEntity(getId())
+              && expectedMessages.isEmpty()) {
+            // NOOP
+          }
+        } finally {
+          ObjectTransitionSafepoint.INSTANCE.register();
         }
-        ObjectTransitionSafepoint.INSTANCE.register();
       }
       return expectedMessages.poll();
     }
 
     private MessageRecord peekExpected() {
       if (expectedMessages.isEmpty()) {
         ObjectTransitionSafepoint.INSTANCE.unregister();
-        while (traceParser.getMoreEventsForEntity(getId()) && expectedMessages.isEmpty()) {
-          // NOOP
+        try {
+          while (traceParser.getMoreEventsForEntity(getId()) && expectedMessages.isEmpty()) {
+            // NOOP
+          }
+        } finally {
+          ObjectTransitionSafepoint.INSTANCE.register();
         }
-        ObjectTransitionSafepoint.INSTANCE.register();
       }
       return expectedMessages.peek();
     }

diff --git a/src/tools/debugger/frontend/Suspension.java b/src/tools/debugger/frontend/Suspension.java
@@ -146,8 +146,17 @@ private void submitTask(final ApplicationThreadTask task) {
   public void suspend() {
     // don't participate in safepoints while being suspended
     ObjectTransitionSafepoint.INSTANCE.unregister();
-    activityThread.markThreadAsSuspendedInDebugger();
 
+    try {
+      activityThread.markThreadAsSuspendedInDebugger();
+      suspendWithoutObjectSafepoints();
+    } finally {
+      activityThread.markThreadAsResumedFromDebugger();
+      ObjectTransitionSafepoint.INSTANCE.register();
+    }
+  }
+
+  public void suspendWithoutObjectSafepoints() {
     boolean continueWaiting = true;
     while (continueWaiting) {
       try {
@@ -174,9 +183,6 @@ public void suspend() {
       suspendedEvent = null;
       stack = null;
     }
-
-    activityThread.markThreadAsResumedFromDebugger();
-    ObjectTransitionSafepoint.INSTANCE.register();
   }
 
   public TracingActivityThread getActivityThread() {