Parser crashing in native code due to multi-threaded access #11121
Assignees
Labels
Milestone
Comments
hubertp
added
p-high
|
Encountered another one: The project has worked a couple of minutes ago with no problems before being re-opened and crashing. |
|
I have reproduced this; it is the same issue as #11104. |
This was referenced Sep 19, 2024
Closed
|
Adding this debugging code: I found this result: It seems the parser state is being concurrently modified by multiple threads. This is not supported. A parser can be moved between threads (with appropriate locks/fencing), or different threads can have their own parsers, but one parser instance must not be concurrently used by multiple threads. |
|
Keziah Wesley reports a new STANDUP for today (2024-09-19): Progress: Investigated parser problems, traced to unsupported sharing of parser instance between threads. It should be finished by 2024-09-26. Next Day: Next day I will be working on the #11121 task. Next task. |
mergify bot
pushed a commit
that referenced
this issue
Sep 20, 2024
- Fix debug logging for #11088 case--attempt to create an exception that is its own cause fails. - In case the parser is used after closing, throw an `IllegalStateException` instead of UB. (This case is not known to occur and doesn't seem to be behind the #11121, but we should handle it more safely if it does.)
|
After a review of parser usage in the backend, I've decided to simplify the parser API to make this kind of bug impossible.
|
kazcw
added a commit
that referenced
this issue
Sep 20, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
kazcw
added a commit
that referenced
this issue
Sep 20, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
kazcw
added a commit
that referenced
this issue
Sep 20, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
kazcw
added a commit
that referenced
this issue
Sep 20, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
kazcw
added a commit
that referenced
this issue
Sep 20, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
kazcw
added a commit
that referenced
this issue
Sep 21, 2024
Stateless (static) parser interface. Buffer-reuse optimization is now hidden behind JNI FFI implementation. Fixes #11121 and prevents similar bugs.
|
It is very interesting result, @kazcw! So far I've been convinced that our execution is only single threaded. We know we want to move towards multi-threaded one - as such fixing the parsing to support multiple threads is desirable. But it is still surprising.
I'd be interested in knowing the stack traces of callers when the collision in the critical section happens. Possibly this small modification of your code could give us traces of the first two threads that collide. diff --git lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java
index 2c375ee840..12290a230b 100644
--- lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java
+++ lib/rust/parser/generate-java/java/org/enso/syntax2/Parser.java
@@ -5,8 +5,12 @@ import java.net.URISyntaxException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.nio.charset.StandardCharsets;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Supplier;
public final class Parser implements AutoCloseable {
+ private final AtomicInteger mutators = new AtomicInteger(0);
+
private static void initializeLibraries() {
try {
System.loadLibrary("enso_parser");
@@ -116,22 +120,39 @@ public final class Parser implements AutoCloseable {
}
public ByteBuffer parseInputLazy(CharSequence input) {
- byte[] inputBytes = input.toString().getBytes(StandardCharsets.UTF_8);
- ByteBuffer inputBuf = ByteBuffer.allocateDirect(inputBytes.length);
- inputBuf.put(inputBytes);
- return parseTreeLazy(state, inputBuf);
+ return criticalSection(
+ () -> {
+ byte[] inputBytes = input.toString().getBytes(StandardCharsets.UTF_8);
+ ByteBuffer inputBuf = ByteBuffer.allocateDirect(inputBytes.length);
+ inputBuf.put(inputBytes);
+ return parseTreeLazy(state, inputBuf);
+ });
}
public Tree parse(CharSequence input) {
- byte[] inputBytes = input.toString().getBytes(StandardCharsets.UTF_8);
- ByteBuffer inputBuf = ByteBuffer.allocateDirect(inputBytes.length);
- inputBuf.put(inputBytes);
- var serializedTree = parseTree(state, inputBuf);
- var base = getLastInputBase(state);
- var metadata = getMetadata(state);
- serializedTree.order(ByteOrder.LITTLE_ENDIAN);
- var message = new Message(serializedTree, input, base, metadata);
- return Tree.deserialize(message);
+ return criticalSection(
+ () -> {
+ byte[] inputBytes = input.toString().getBytes(StandardCharsets.UTF_8);
+ ByteBuffer inputBuf = ByteBuffer.allocateDirect(inputBytes.length);
+ inputBuf.put(inputBytes);
+ var serializedTree = parseTree(state, inputBuf);
+ var base = getLastInputBase(state);
+ var metadata = getMetadata(state);
+ serializedTree.order(ByteOrder.LITTLE_ENDIAN);
+ var message = new Message(serializedTree, input, base, metadata);
+ return Tree.deserialize(message);
+ });
+ }
+
+ private <R> R criticalSection(Supplier<R> action) {
+ if (mutators.getAndIncrement() != 0) {
+ throw new IllegalStateException("Race condition detected. On enter.");
+ }
+ var r = action.get();
+ if (mutators.getAndDecrement() != 1) {
+ throw new IllegalStateException("Race condition detected. On exit.");
+ }
+ return r;
}
public static String getWarningMessage(Warning warning) {then there is going to be a lot of warnings, as the counters will be off. Or maybe |
|
@JaroslavTulach Added more detailed diagnostics based on your suggestion, and I found this thread conflict: |
|
It seems visualization expression compilation and module compilation use the same |
JaroslavTulach
changed the title
Amazing! Thanks a lot, @kazcw. So, it is
That's a very good question. So far the common expectation among @4e6, @Akirathan, @hubertp was that compilation is single-threaded. Apparently it is not. Yes, it can have consequences.
|
jdunkerley
pushed a commit
that referenced
this issue
Sep 26, 2024
- Fix debug logging for #11088 case--attempt to create an exception that is its own cause fails. - In case the parser is used after closing, throw an `IllegalStateException` instead of UB. (This case is not known to occur and doesn't seem to be behind the #11121, but we should handle it more safely if it does.) (cherry picked from commit e587d56)
github-project-automation
bot
moved this from 👁️ Code review
to 🟢 Accepted
in Issues Board
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
or
hs_err_pid90180.log
hs_err_pid90473.log
The text was updated successfully, but these errors were encountered: