Allow polyglot embedders to control `throwDeniedThreadAccess` #8266

JaroslavTulach · 2024-01-29T05:26:36Z

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.

…internal lock

JaroslavTulach · 2024-01-30T03:35:43Z

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 · 2024-01-30T10:50:58Z

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 · 2024-02-05T10:53:13Z

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 · 2024-02-12T12:09:07Z

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 · 2024-03-01T04:34:00Z

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.

chumer · 2024-01-30T09:47:37Z

truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/EngineAccessor.java

@@ -1150,7 +1150,14 @@ public Thread createThread(Object polyglotLanguageContext, Runnable runnable, Ob
                threadContext = innerContext.getContext(threadContext.language);
            }
            PolyglotThread newThread = new PolyglotThread(threadContext, runnable, group, stackSize, beforeEnter, afterLeave);
-            threadContext.context.checkMultiThreadedAccess(newThread);


Can't you move the loop into checkMultiThreadedAccess?

chumer · 2024-03-18T12:41:13Z

...cle.truffle.api.test/src/com/oracle/truffle/api/test/polyglot/MultiThreadedLanguageTest.java

+            fail();
+        } catch (PolyglotException e) {
+            assertTrue(e.getMessage().contains("SecurityException"));
+            assertTrue(e.isInternalError());


Internal errors must never be reachable. Whatever exception is thrown by onDeniedThreadAccess must be a host exception.

chumer · 2024-03-18T12:50:54Z

...cle.truffle.api.test/src/com/oracle/truffle/api/test/polyglot/MultiThreadedLanguageTest.java

+    }
+
+    @Test
+    public void testNoThreadAllowedRetry() {


I would like to see an integration test that uses it in a way that involves multiple threads.
Something that really models how you want to use it.

How about something that would deadlock if executed within the lock?

chumer · 2024-03-18T12:51:35Z

...fle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotContextImpl.java

-                synchronized (this) {
-                    PolyglotThreadInfo threadInfo = getCurrentThreadInfo();
+                PolyglotThreadAccessException threadAccessException = null;
+                LOOP: for (;;) {


minor: We are not actually using upper-case loop labels. We typically use lower-cased ones. Let's be consistent.

chumer · 2024-03-18T12:53:29Z

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/Context.java

+         * yielded by default. By installing this {@code handler} one can control what shall happen.
+         * Either to throw the provided exception or resolve the multithreaded situation and return
+         * to retry the thread access again.
+         *


This could use usage examples. For example customization of the error message and or using a locking strategy?

chumer · 2024-03-18T12:53:47Z

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/Context.java

+         * @param handler callback that either throws the provided {@code RuntimeException} or
+         *            returns to signal request for retry
+         * @return this builder
+         * @since 23.2


Target version is 24.1

chumer · 2024-03-18T12:53:59Z

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/Context.java

+         * @return this builder
+         * @since 23.2
+         */
+        public Builder onDeniedThreadAccess(Consumer<RuntimeException> handler) {


API changes like this need an entry in the changelog.

chumer · 2024-03-18T12:57:00Z

...m.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotThreadAccessException.java

+ */
+package com.oracle.truffle.polyglot;
+
+final class PolyglotThreadAccessException extends Exception {


This exception does not need a stack trace as it is supposed to be caught always.

chumer · 2024-03-18T13:01:24Z

sdk/src/org.graalvm.polyglot/src/org/graalvm/polyglot/Context.java

@@ -1180,6 +1182,23 @@ public Builder allowCreateThread(boolean enabled) {
            return this;
        }

+        /**


The corresponding TruffleContext API is missing.

Avoid throwing PolyglotEngineException.illegalState while holding an …

82b14c3

…internal lock

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 29, 2024

Jaroslav Tulach added 4 commits January 29, 2024 06:45

Pass in the right PolyglotContextImpl

6133eb0

Allow registration of onDeniedThreadAccess callback

c4e3624

Break the loop when the check throws no exception

fe1c98f

mx eclipseformat

853702c

This was referenced Jan 29, 2024

Using latest GraalVM bits in Enso enso-org/enso#8885

Closed

Let languages control multi threaded access ... is not allowed for language(s) js #8033

Open

enso-bot bot mentioned this pull request Jan 30, 2024

Dangling expression should throw a compilation error enso-org/enso#8832

Closed

JaroslavTulach mentioned this pull request Jan 30, 2024

Run Enso tests with assertions enabled enso-org/enso#7882

Merged

2 tasks

fniephaus requested a review from chumer January 30, 2024 21:18

fniephaus assigned chumer and JaroslavTulach Jan 30, 2024

enso-bot bot mentioned this pull request Jan 31, 2024

Use single cache file per library enso-org/enso#8639

Closed

Jaroslav Tulach added 3 commits February 12, 2024 12:52

onDeniedThreadAccess callback can change the exception test

addb748

Formatting Javadoc comments

b2d6f34

onDeniedThreadAccess can return to request a retry

f91faeb

Removing empty finally block

22746a8

enso-bot bot mentioned this pull request Feb 13, 2024

Unresolved constructor in a case statement seems to lack a correlated source reference enso-org/enso#8976

Closed

chumer requested changes Mar 18, 2024

View reviewed changes

chumer requested a review from jchalou March 18, 2024 13:03

enso-bot bot mentioned this pull request Mar 19, 2024

Generate completion of Table.join join criteria using data from both joined tables enso-org/enso#5629

Closed

JaroslavTulach mentioned this pull request Mar 26, 2024

Run ydoc-server with GraalVM enso-org/enso#9528

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow polyglot embedders to control `throwDeniedThreadAccess` #8266

Allow polyglot embedders to control `throwDeniedThreadAccess` #8266

JaroslavTulach commented Jan 29, 2024 •

edited

JaroslavTulach commented Jan 30, 2024 •

edited

chumer commented Jan 30, 2024

JaroslavTulach commented Feb 5, 2024 •

edited

JaroslavTulach commented Feb 12, 2024

JaroslavTulach commented Mar 1, 2024

chumer left a comment

chumer Jan 30, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

chumer Mar 18, 2024

Allow polyglot embedders to control throwDeniedThreadAccess #8266

Are you sure you want to change the base?

Allow polyglot embedders to control throwDeniedThreadAccess #8266

Conversation

JaroslavTulach commented Jan 29, 2024 • edited

JaroslavTulach commented Jan 30, 2024 • edited

chumer commented Jan 30, 2024

JaroslavTulach commented Feb 5, 2024 • edited

JaroslavTulach commented Feb 12, 2024

JaroslavTulach commented Mar 1, 2024

chumer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Allow polyglot embedders to control `throwDeniedThreadAccess` #8266

Allow polyglot embedders to control `throwDeniedThreadAccess` #8266

JaroslavTulach commented Jan 29, 2024 •

edited

JaroslavTulach commented Jan 30, 2024 •

edited

JaroslavTulach commented Feb 5, 2024 •

edited