Allow control of throwDeniedThreadAccess via TruffleContext.threadAccessDeniedHandler

[JaroslavTulach]

Let's fix #8033 by controlling what happens when throwDeniedThreadAccess is called.

The simplest way to address #8033 is to replace the current action in throwDeniedThreadAccess - e.g. throw PolyglotEngineException.illegalState with a callback to embedder to decide what to do - don't throw, but retry. However calling foreign code at the moment of throwDeniedThreadAccess is slightly dangerous as internal lock on PolyglotContextImpl is being held. As such, let's start with the first step 82b14c3 - let's make sure the exception is always thrown without holding internal lock(s). To do so, I am introducing an internal checked exception and propagating it up to the point where the lock isn't held. Only then it is converted to original PolyglotEngineException.illegalState exception and thrown. The fact that the exception is checked, instructs the javac to check all paths that may yield the PolyglotThreadAccessException and help me to never forget to handle the conversion. The unusual loop: for (;;) {... continue loop.... break loop} style is used as a preparation for the next step...

The next step then defines Builder.onDeniedThreadAccess(Consumer<RuntimeException> handler) which allows the embedders to decide whether to throw the exception or do something more intrinsic.

The third step is to use the API. Great, it seems to work as planned.

[JaroslavTulach]

With onDeniedThreadAccess as proposed by this PR the embedders have a way to act like an Ethernet on collision and successfully orchestrate two threads accessing the same context to run into completion.

More info on Ethernet collision detection and behavior - most important "Calculate and wait the random backoff".

The good thing from the Truffle API perspective is:

• the ugly Ethernet like behavior isn't part of Truffle code base
• people can use different style - try to access GIL instead of random waiting - etc.
• the Truffle API doesn't prescribe the correct style, just opens the door to coordinate the access somehow
• all that is possible by this simple onDeniedThreadAccess callback

The onDeniedThreadAccess callback is an API seam that allows accepting something that would otherwise be unacceptable. All the ugliness remains in the hands of embedders...

[chumer]

I like the idea in principle.

What I like about this approach:

• Does not need any checks on the fast-path.
• Flexibility to implement your own strategy in case of multi-threaded access without allowing actual multi-threaded access.
• Careful considerations of current locking strategy.

What I don't like about this approach:

• It makes already quite complex control flow even more complex. I think some of the changes could be improved for better readability.
• It only works if you exclusively use single-threaded languages. Note that we force transition to multi-threaded as soon as language is initialized that depends on it to avoid late deoptimization. We would need to make sure we would need to change:

     boolean transitionToMultiThreading = isSingleThreaded() && hasActiveOtherThread(true, false);

to something like this (just a sketch)

     boolean multiThreadedAccess = (isSingleThreaded() || multiThreadedCallback) &&  hasActiveOtherThread(true, false);

• Following up the previous problem I think we should change the API to be a callback whenever a multi-threaded access is performed. (e.g. Builder#onMultiThreadedAccess)
• We lock ourselves in with the current approach of doing slow-path and fast-path thread transitions. If we ever want to have fast transitions (enter/leave) from multiple threads this will be harder to do as we need to keep supporting the callback.
• Further investigation and testing needed for toggling behavior. What if multiple threads try to enter/leave concurrently. Can it happen that no thread ever wins?
• Its a complex API to use correctly. Lots of traps there. So we need extensive documentation of dos and don'ts. Ideally also examples on how to use it.
• We also need to extend the TruffleContext API in a complete implementation.

[JaroslavTulach]

I like the idea in principle.

OK, I'll write some unit tests and then we can continue the review. I am not sure I fully understand all the comments, for example:

only works if you exclusively use single-threaded languages

Sure, I only care about Graal.js - all the other languages we are interested in are mutli threading ready. All I need is to make sure when there is a collision, it doesn't end up that fatal as right now and there is a way to recover. I don't get the comment about multi-threaded languages. We can sort it out when I prepare some unit tests for review. But I'd prefer to keep my PR as minimal as possible - solving complete Truffle corner cases is job for a professional, not for an outsider like me.

quite complex control flow even more complex

Do you mean those for (;;) { ... continue ... } loops? They look more scary in the diff than in reality. I just had to shift the indentation and that makes the change look bigger than it is.

I'll find some time this week and add some test cases to this PR.

[JaroslavTulach]

The first test added in addb748 verifies that all the ways that originally yielded illegalState exception can reach the onDeniedThreadAccess callback which can then throw some other exception.

The other test (added in f91faeb) verifies that the onDeniedThreadAccess callback can request a retry and then the threaded access succeeds.

@chumer, what additional tests you want me to provide?

[JaroslavTulach]

Hello @chumer, is there anything else I shall do to get this API change in?

[chumer]

A great step forward. Some of the things I previously commented on were unfortunately not adressed. Added a few more comments.

[JaroslavTulach]

I tried to use this change in Enso. I am struggling. Enso still uses 24.0.0 - upgrading to 24.2.0-SNAPSHOT doesn't seem to be easy at all (I have to give up otherwise I don't make it for GraalVM meetup). Backporting the change in this PR back to 24.0.0 isn't without issues either. But overall I think: Enso needs just the TruffleContext change. Following patch seems to trigger the right bits:

diff --git engine/runtime-fat-jar/src/main/java/module-info.java engine/runtime-fat-jar/src/main/java/module-info.java
index 87816c9723..9d577e04bb 100644
--- engine/runtime-fat-jar/src/main/java/module-info.java
+++ engine/runtime-fat-jar/src/main/java/module-info.java
@@ -9,6 +9,7 @@ open module org.enso.runtime {
   // works.
   requires org.enso.profiling;
   requires org.enso.ydoc;
+  requires org.graalvm.collections;
   requires org.graalvm.polyglot;
   requires org.graalvm.truffle;
   requires static org.slf4j;
diff --git engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ForeignMethodInvokeTest.java engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ForeignMethodInvokeTest.java
index 3cb3722f50..6a2822161e 100644
--- engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ForeignMethodInvokeTest.java
+++ engine/runtime-integration-tests/src/test/java/org/enso/interpreter/test/ForeignMethodInvokeTest.java
@@ -13,7 +13,6 @@ import org.graalvm.polyglot.Context;
 import org.graalvm.polyglot.Value;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
 import org.junit.Test;

 public class ForeignMethodInvokeTest {
@@ -91,7 +90,6 @@ public class ForeignMethodInvokeTest {
     assertEquals(12, res2.getArrayElement(2).asInt());
   }

-  @Ignore
   @Test
   public void testParallelInteropWithJavaScript() throws Exception {
     var source =
diff --git engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java
index 0eed392545..929818ed01 100644
--- engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java
+++ engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java
@@ -44,6 +44,7 @@ final class EpbContext {
       if (innerContext == null) {
         innerContext =
             env.newInnerContextBuilder()
+                .onDeniedThreadAccess(this::deniedThreadAccess)
                 .initializeCreatorContext(true)
                 .inheritAllAccess(true)
                 .config(INNER_OPTION, "yes")
@@ -52,6 +53,14 @@ final class EpbContext {
     }
   }

+  private void deniedThreadAccess(Throwable t) {
+    try {
+      Thread.sleep(10);
+    } catch (InterruptedException ex) {
+      // go on
+    }
+  }
+
   /**
    * @param node the location of context access. Pass {@code null} if not in a node.
    * @return the proper context instance for the current {@link

I am going to remove the API changes in org.graalvm.polyglot.Context API.

[JaroslavTulach]

@chumer, I have addressed all your API change requests. Please review again. Right now I am struggling with endless loop in

graal/truffle$ mx build && mx unittest TruffleContextTest

It is a result of a bad merge. I'll address it soon. However I don't expect it to impact the observable API - e.g. you can review even now.

[chumer]

minor changes requested

[JaroslavTulach]

Change log updated in 7149f99

[JaroslavTulach]

Squashed into a single commit in 85854c0

[jchalou]

Jaroslav, I am about to merge your work. In case you want your name on it, please resolve conflict, apply the following diff:

diff --git a/truffle/src/com.oracle.truffle.api.test/src/META-INF/native-image/reflect-config.json b/truffle/src/com.oracle.truffle.api.test/src/META-INF/native-image/reflect-config.json
index d0fd869ca63..513287d1b37 100644
--- a/truffle/src/com.oracle.truffle.api.test/src/META-INF/native-image/reflect-config.json
+++ b/truffle/src/com.oracle.truffle.api.test/src/META-INF/native-image/reflect-config.json
@@ -261,5 +261,9 @@
   {
     "name": "java.nio.HeapByteBuffer",
     "allPublicMethods": true
+  },
+  {
+    "name": "org.hamcrest.core.StringContains",
+    "allPublicMethods": true
   }
 ]
diff --git a/truffle/src/com.oracle.truffle.api/snapshot.sigtest b/truffle/src/com.oracle.truffle.api/snapshot.sigtest
index edd6237c979..662eec86790 100644
--- a/truffle/src/com.oracle.truffle.api/snapshot.sigtest
+++ b/truffle/src/com.oracle.truffle.api/snapshot.sigtest
@@ -325,9 +325,10 @@ meth public com.oracle.truffle.api.TruffleContext$Builder onExited(java.util.fun
 meth public com.oracle.truffle.api.TruffleContext$Builder option(java.lang.String,java.lang.String)
 meth public com.oracle.truffle.api.TruffleContext$Builder options(java.util.Map<java.lang.String,java.lang.String>)
 meth public com.oracle.truffle.api.TruffleContext$Builder out(java.io.OutputStream)
+meth public com.oracle.truffle.api.TruffleContext$Builder threadAccessDeniedHandler(java.util.function.Consumer<java.lang.String>)
 meth public com.oracle.truffle.api.TruffleContext$Builder timeZone(java.time.ZoneId)
 supr java.lang.Object
-hfds allowCreateProcess,allowCreateThread,allowEnvironmentAccess,allowHostClassLoading,allowHostLookup,allowIO,allowInnerContextOptions,allowNativeAccess,allowPolyglotAccess,arguments,config,environment,err,in,inheritAccess,initializeCreatorContext,onCancelled,onClosed,onExited,options,out,permittedLanguages,sharingEnabled,sourceEnvironment,timeZone
+hfds allowCreateProcess,allowCreateThread,allowEnvironmentAccess,allowHostClassLoading,allowHostLookup,allowIO,allowInnerContextOptions,allowNativeAccess,allowPolyglotAccess,arguments,config,environment,err,in,inheritAccess,initializeCreatorContext,onCancelled,onClosed,onExited,options,out,permittedLanguages,sharingEnabled,sourceEnvironment,threadAccessDeniedHandler,timeZone
 
 CLSS public final com.oracle.truffle.api.TruffleFile
 fld public final static com.oracle.truffle.api.TruffleFile$AttributeDescriptor<java.lang.Boolean> IS_DIRECTORY
diff --git a/truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java b/truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java
index dad43965ae6..3084cd30d32 100644
--- a/truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java
+++ b/truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java
@@ -822,6 +822,7 @@ final class PolyglotContextImpl implements com.oracle.truffle.polyglot.PolyglotI
                     boolean leaveAndEnter) {
         List<Map.Entry<Thread, PolyglotThreadInfo>> deadThreads = null;
         PolyglotThreadInfo enteredThread = null;
+        boolean localEnterReverted = enterReverted;
         Object[] prev = null;
         Thread current = Thread.currentThread();
         try {
@@ -844,7 +845,7 @@ final class PolyglotContextImpl implements com.oracle.truffle.polyglot.PolyglotI
                         synchronized (this) {
                             PolyglotThreadInfo threadInfo = getCurrentThreadInfo();
 
-                            if (enterReverted && threadInfo.getEnteredCount() == 0) {
+                            if (localEnterReverted && threadInfo.getEnteredCount() == 0) {
                                 threadLocalActions.notifyThreadActivation(threadInfo, false);
                                 if ((state.isCancelling() || state.isExiting() || state == State.CLOSED_CANCELLED || state == State.CLOSED_EXITED) && !threadInfo.isActive()) {
                                     notifyThreadClosed(threadInfo);
@@ -857,6 +858,7 @@ final class PolyglotContextImpl implements com.oracle.truffle.polyglot.PolyglotI
                                     notifyAll();
                                 }
                             }
+                            localEnterReverted = false;
                             if (deactivateSafepoints && threadInfo != PolyglotThreadInfo.NULL) {
                                 threadLocalActions.notifyThreadActivation(threadInfo, false);
                             }

and then squash into one commit. If you don't care about your name being on this, I can do all that for you. Please let me know soon, thanks.

[JaroslavTulach]

Jaroslav, I am about to merge your work. In case you want your name on it, please resolve conflict, apply the following diff:

It is not enough to apply the patch. Conflict still remains. I had to add a810ee8 - hopefully it is the last time I need to manually merge conflicts in PolyglotContextImpl.

Finally squashed in b7f3b4c

[jchalou]

Jaroslav, I am about to merge your work. In case you want your name on it, please resolve conflict, apply the following diff:

It is not enough to apply the patch. Conflict still remains. I had to add a810ee8 - hopefully it is the last time I need to manually merge conflicts in PolyglotContextImpl.

Finally squashed in b7f3b4c

Thanks, the patch was not intended to resolve the conflict, these we just further changes that needed to be applied after the conflict was resolved.

[JaroslavTulach]

I see the threadAccessDeniedHandler in the official Javadoc. Thank you for your help accepting my patch!