Make the alarm callback path thread-safe, roll back failed registrations, and stop losing callLater failures

javatools/src/main/java/org/xvm/runtime/ServiceContext.java

@@ -6,7 +6,6 @@
 
 import java.util.Arrays;
 import java.util.EnumMap;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Queue;
@@ -273,19 +272,8 @@ private Map<TransientId, ObjectHandle> ensureTransientMap() {
      *
      * @return the map of callbacks keyed by unique ids
      */
-    protected Map<Long, WeakCallback.Callback> ensureCallbackMap() {
-        Map<Long, WeakCallback.Callback> map = m_mapCallbacks;
-        if (map == null) {
-            map = m_mapCallbacks = new HashMap<>();
-        }
-        return map;
-    }
-
-    /**
-     * @return the map of callbacks keyed by unique ids
-     */
     protected Map<Long, WeakCallback.Callback> getCallbackMap() {
-        return m_mapCallbacks;
+        return f_mapCallbacks;
     }
 
     // ----- scheduling  ---------------------------------------------------------------------------
@@ -1030,7 +1018,7 @@ public CompletableFuture<ObjectHandle> callLater(FunctionHandle hFunction, Objec
         if (future != null) {
             future.whenComplete((r, x) -> {
                 if (x != null) {
-                    callUnhandledExceptionHandler(((WrapperException) x).getExceptionHandle());
+                    reportUnhandledException(x);
                 }
             });
         }
@@ -1053,7 +1041,7 @@ public CompletableFuture<ObjectHandle> callLater(Frame frame, FunctionHandle hFu
         if (future != null) {
             future.whenComplete((r, x) -> {
                 if (x != null) {
-                    callUnhandledExceptionHandler(((WrapperException) x).getExceptionHandle());
+                    reportUnhandledException(x);
                 }
             });
         }
@@ -1505,6 +1493,34 @@ public String toString() {
         return request.f_future;
     }
 
+    /**
+     * Report a failed "callLater" to the unhandled exception handler.
+     * <p/>
+     * This runs inside a {@link CompletableFuture} completion stage, which discards anything thrown
+     * out of it - so nothing here may assume what the failure is, and nothing here may throw. The
+     * previous code cast the throwable straight to {@link WrapperException}: any other failure
+     * turned into a ClassCastException that the completion stage swallowed, taking the original
+     * failure with it and leaving the handler uncalled. A reported failure became a silent one.
+     *
+     * @param e  the throwable the future completed with; never null
+     */
+    private void reportUnhandledException(Throwable e) {
+        try {
+            // translate() unwraps CompletionException/ExecutionException, maps a WrapperException
+            // to its handle, and renders anything else - including a cancellation, an interrupt, or
+            // a native failure - as a visible exception rather than discarding it
+            ExceptionHandle hException = Utils.translate(e);
+            if (hException != null) {
+                callUnhandledExceptionHandler(hException);
+            }
+        } catch (Throwable eReport) {
+            // must not happen, and must not escape into the completion stage that would swallow it
+            System.err.println("Unexpected failure reporting an unhandled exception: " + f_sName);
+            eReport.printStackTrace(System.err);
+            e.printStackTrace(System.err);
+        }
+    }
+
     protected void callUnhandledExceptionHandler(ExceptionHandle hException) {
         FunctionHandle hFunction = m_hExceptionHandler;
         if (hFunction == null) {
@@ -2191,9 +2207,12 @@ public enum ServiceStatus {
     private Map<TransientId, ObjectHandle> m_mapTransient;
 
     /**
-     * A "service-local" cache for service callbacks.
+     * The registered service callbacks. The map is deliberately eager, final, and concurrent: the
+     * owning service registers callbacks on its own thread, but alarm maturation extracts them on
+     * the shared native timer thread and alarm cancellation discards them from natural code, so
+     * this registry is mutated from multiple threads by design.
      */
-    private Map<Long, WeakCallback.Callback> m_mapCallbacks;
+    private final Map<Long, WeakCallback.Callback> f_mapCallbacks = new ConcurrentHashMap<>();
 
     /**
      * A wake-up scheduler to process registered timeouts.

javatools/src/main/java/org/xvm/runtime/WeakCallback.java

@@ -18,21 +18,36 @@ public WeakCallback(Frame frame, FunctionHandle hFunction) {
         super(frame.f_context);
 
         f_lCallbackId = frame.f_context.f_container.f_runtime.makeUniqueId();
-        frame.f_context.ensureCallbackMap().put(f_lCallbackId, new Callback(frame, hFunction));
+        frame.f_context.getCallbackMap().put(f_lCallbackId, new Callback(frame, hFunction));
     }
 
     /**
-     * @return the underlying function; never null
+     * Extract the callback data, removing it from the owning service's registry.
+     * <p/>
+     * This runs on the shared native timer thread. A missing callback is a normal outcome - the
+     * service may have been collected, or the alarm may have been discarded by a racing cancel -
+     * and must never throw here: an exception escaping a {@link java.util.TimerTask} kills the
+     * shared static {@link java.util.Timer} and silently disables every alarm in every container.
+     *
+     * @return the callback data, or null if the service is gone or the callback was already
+     *         extracted or discarded
      */
     public Callback extractCallback() {
+        ServiceContext context = get();
+        return context == null
+                ? null
+                : context.getCallbackMap().remove(f_lCallbackId);
+    }
+
+    /**
+     * Discard the callback data without running it. Called when an alarm is canceled, so that the
+     * registry does not leak the captured frame and function for the lifetime of the service.
+     */
+    public void discard() {
         ServiceContext context = get();
         if (context != null) {
-            Callback callback = context.getCallbackMap().remove(f_lCallbackId);
-            if (callback != null) {
-                return callback;
-            }
+            context.getCallbackMap().remove(f_lCallbackId);
         }
-        throw new IllegalStateException();
     }
 
     @Override

javatools/src/main/java/org/xvm/runtime/template/_native/temporal/xLocalClock.java

@@ -128,9 +128,10 @@ private int invokeSchedule(Frame frame, long ldtWakeup, long cDelay,
 
         Alarm alarm = new Alarm(new WeakCallback(frame, hAlarm), ldtWakeup, hKeepAlive.get());
         try {
+            alarm.registerKeepAlive();
             TIMER.schedule(alarm.getTrigger(), cDelay);
         } catch (Exception e) {
-            alarm.cancel();
+            alarm.cancelAfterScheduleFailure();
             return frame.raiseException(e.getMessage());
         }
 
@@ -200,11 +201,7 @@ protected Alarm(WeakCallback refCallback, long ldtWakeUp, boolean fKeepAlive) {
             f_refCallback = refCallback;
             f_ldtWakeup   = ldtWakeUp;
             m_trigger     = new Trigger(this);
-            f_Registered  = fKeepAlive;
-
-            if (fKeepAlive) {
-                refCallback.get().f_container.registerNativeCallback();
-            }
+            f_fKeepAlive  = fKeepAlive;
         }
 
         /**
@@ -214,6 +211,35 @@ public Trigger getTrigger() {
             return m_trigger;
         }
 
+        /**
+         * Claim container keep-alive ownership as part of the schedule attempt rather than from the
+         * constructor. Registering in the constructor left the count elevated forever when
+         * Timer.schedule(...) failed, since TimerTask.cancel() reports false for a task that was
+         * never scheduled and the old cancel() path therefore skipped the unregister.
+         */
+        public void registerKeepAlive() {
+            if (!f_fKeepAlive) {
+                return;
+            }
+
+            ServiceContext context = f_refCallback.get();
+            if (context == null) {
+                return;
+            }
+
+            Container container = context.f_container;
+            synchronized (this) {
+                if (m_fFinished || m_containerRegistered != null) {
+                    return;
+                }
+
+                // remember the exact owner while the callback is registered: the WeakCallback can
+                // legitimately clear before cleanup, but keep-alive ownership must still unwind
+                container.registerNativeCallback();
+                m_containerRegistered = container;
+            }
+        }
+
         /**
          * Called when the alarm is triggered by the Java timer.
          */
@@ -231,30 +257,70 @@ public void run() {
 
                 long ldtNow = container.currentTimeMillis();
                 if (ldtNow >= f_ldtWakeup) {
-                    WeakCallback.Callback callback = f_refCallback.extractCallback();
-                    context.callLater(callback.frame(), callback.functionHandle(), Utils.OBJECTS_NONE);
-                    if (f_Registered) {
-                        container.unregisterNativeCallback();
+                    Container containerRegistered = finish();
+                    try {
+                        WeakCallback.Callback callback = f_refCallback.extractCallback();
+                        if (callback != null) {
+                            context.callLater(callback.frame(), callback.functionHandle(),
+                                    Utils.OBJECTS_NONE);
+                        }
+                    } finally {
+                        unregister(containerRegistered);
                     }
                 } else {
                     // reschedule
                     TIMER.schedule(m_trigger = new Trigger(this), f_ldtWakeup - ldtNow);
                 }
+            } else {
+                // the owning service is gone; release the keep-alive count we still hold
+                unregister(finish());
             }
         }
 
         /**
          * Called when the alarm is canceled by the natural code.
          */
         public boolean cancel() {
-            boolean        fCancelled = m_trigger.cancel();
-            ServiceContext context    = f_refCallback.get();
-            if (context != null && fCancelled && f_Registered) {
-                context.f_container.unregisterNativeCallback();
+            boolean fCancelled = m_trigger.cancel();
+            if (fCancelled) {
+                // the trigger will never run, so the registry entry would otherwise leak the
+                // captured frame and function for the lifetime of the service
+                f_refCallback.discard();
+                unregister(finish());
             }
             return fCancelled;
         }
 
+        /**
+         * Roll back a failed schedule attempt. The alarm has not escaped to natural code yet, so it
+         * can be marked finished even though TimerTask.cancel() reports false for a task that was
+         * never scheduled.
+         */
+        public void cancelAfterScheduleFailure() {
+            m_trigger.cancel();
+            f_refCallback.discard();
+            unregister(finish());
+        }
+
+        /**
+         * Mark this alarm done and hand off the keep-alive ownership, if any, exactly once.
+         *
+         * @return the container this alarm registered a keep-alive callback with, or null
+         */
+        private synchronized Container finish() {
+            m_fFinished = true;
+
+            Container container = m_containerRegistered;
+            m_containerRegistered = null;
+            return container;
+        }
+
+        private static void unregister(Container container) {
+            if (container != null) {
+                container.unregisterNativeCallback();
+            }
+        }
+
         /**
          * A TimerTask that is scheduled on the Java timer and is used to trigger the alarm.
          */
@@ -274,8 +340,10 @@ public void run() {
 
         private final WeakCallback f_refCallback;
         private final long         f_ldtWakeup;
-        private final boolean      f_Registered;
+        private final boolean      f_fKeepAlive;
         private       Trigger      m_trigger;
+        private       Container    m_containerRegistered;
+        private       boolean      m_fFinished;
     }
 
     // ----- constants and fields ------------------------------------------------------------------