Skip to content

Commit 2aafb77

Browse files
otethalotethal
committed
[GR-30335] Fixed GIL acquire issues related to StackOverflowError
PullRequest: graalpython/1812
2 parents 2ceea7f + f810f67 commit 2aafb77

File tree

5 files changed

+74
-46
lines changed

5 files changed

+74
-46
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/WeakRefModuleBuiltins.java

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242

4343
import java.lang.ref.Reference;
4444
import java.lang.ref.ReferenceQueue;
45+
import java.util.ArrayList;
4546
import java.util.List;
4647

4748
import com.oracle.graal.python.builtins.Builtin;
@@ -90,20 +91,28 @@ protected List<? extends NodeFactory<? extends PythonBuiltinBaseNode>> getNodeFa
9091
}
9192

9293
private static class WeakrefCallbackAction extends AsyncHandler.AsyncPythonAction {
93-
private final WeakRefStorage reference;
94+
private final WeakRefStorage[] references;
95+
private int index;
9496

95-
public WeakrefCallbackAction(PReferenceType.WeakRefStorage reference) {
96-
this.reference = reference;
97+
public WeakrefCallbackAction(WeakRefStorage[] weakRefStorages) {
98+
this.references = weakRefStorages;
99+
this.index = 0;
97100
}
98101

99102
@Override
100103
public Object callable() {
101-
return reference.getCallback();
104+
return references[index].getCallback();
102105
}
103106

104107
@Override
105108
public Object[] arguments() {
106-
return new Object[]{reference.getRef()};
109+
return new Object[]{references[index].getRef()};
110+
}
111+
112+
@Override
113+
public boolean proceed() {
114+
index++;
115+
return index < references.length;
107116
}
108117
}
109118

@@ -124,11 +133,17 @@ public void postInitialize(PythonCore core) {
124133
} catch (InterruptedException e) {
125134
Thread.currentThread().interrupt();
126135
}
127-
if (reference instanceof PReferenceType.WeakRefStorage) {
128-
return new WeakrefCallbackAction((PReferenceType.WeakRefStorage) reference);
129-
} else {
130-
return null;
136+
ArrayList<PReferenceType.WeakRefStorage> refs = new ArrayList<>();
137+
do {
138+
if (reference instanceof PReferenceType.WeakRefStorage) {
139+
refs.add((PReferenceType.WeakRefStorage) reference);
140+
}
141+
reference = weakRefQueue.poll();
142+
} while (reference != null);
143+
if (!refs.isEmpty()) {
144+
return new WeakrefCallbackAction(refs.toArray(new PReferenceType.WeakRefStorage[0]));
131145
}
146+
return null;
132147
});
133148
}
134149

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/control/TopLevelExceptionHandler.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,7 @@ public Object execute(VirtualFrame frame) {
136136
assert !PArguments.isPythonFrame(frame);
137137
throw handlePythonException(e.getEscapedException());
138138
} catch (StackOverflowError e) {
139+
getContext().reacquireGilAfterStackOverflow();
139140
PBaseException newException = PythonObjectFactory.getUncached().createBaseException(RecursionError, "maximum recursion depth exceeded", new Object[]{});
140141
PException pe = ExceptionHandlingStatementNode.wrapJavaException(e, this, newException);
141142
throw handlePythonException(pe.getEscapedException());

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/statement/ExceptionHandlingStatementNode.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ protected final PException wrapJavaExceptionIfApplicable(Throwable e) {
209209
return wrapJavaException(e, this, factory().createBaseException(SystemError, "%m", new Object[]{e}));
210210
}
211211
if (e instanceof StackOverflowError) {
212+
getContext().reacquireGilAfterStackOverflow();
212213
return wrapJavaException(e, this, factory().createBaseException(RecursionError, "maximum recursion depth exceeded", new Object[]{}));
213214
}
214215
return null;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/AsyncHandler.java

Lines changed: 27 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -109,27 +109,37 @@ protected int frameIndex() {
109109
return -1;
110110
}
111111

112+
/**
113+
* As long as a subclass wants to run multiple callables in a single action, it can return
114+
* {@code true} here.
115+
*/
116+
protected boolean proceed() {
117+
return false;
118+
}
119+
112120
@Override
113121
public final void execute(PythonContext context) {
114-
Object callable = callable();
115-
if (callable != null) {
116-
Object[] arguments = arguments();
117-
Object[] args = PArguments.create(arguments.length + CallRootNode.ASYNC_ARG_COUNT);
118-
PythonUtils.arraycopy(arguments, 0, args, PArguments.USER_ARGUMENTS_OFFSET + CallRootNode.ASYNC_ARG_COUNT, arguments.length);
119-
PArguments.setArgument(args, CallRootNode.ASYNC_CALLABLE_INDEX, callable);
120-
PArguments.setArgument(args, CallRootNode.ASYNC_FRAME_INDEX_INDEX, frameIndex());
122+
do {
123+
Object callable = callable();
124+
if (callable != null) {
125+
Object[] arguments = arguments();
126+
Object[] args = PArguments.create(arguments.length + CallRootNode.ASYNC_ARG_COUNT);
127+
PythonUtils.arraycopy(arguments, 0, args, PArguments.USER_ARGUMENTS_OFFSET + CallRootNode.ASYNC_ARG_COUNT, arguments.length);
128+
PArguments.setArgument(args, CallRootNode.ASYNC_CALLABLE_INDEX, callable);
129+
PArguments.setArgument(args, CallRootNode.ASYNC_FRAME_INDEX_INDEX, frameIndex());
121130

122-
try {
123-
GenericInvokeNode.getUncached().execute(context.getAsyncHandler().callTarget, args);
124-
} catch (RuntimeException e) {
125-
// we cannot raise the exception here (well, we could, but CPython
126-
// doesn't), so we do what they do and just print it
127-
128-
// Just print a Python-like stack trace; CPython does the same (see
129-
// 'weakrefobject.c: handle_callback')
130-
ExceptionUtils.printPythonLikeStackTrace(e);
131+
try {
132+
GenericInvokeNode.getUncached().execute(context.getAsyncHandler().callTarget, args);
133+
} catch (RuntimeException e) {
134+
// we cannot raise the exception here (well, we could, but CPython
135+
// doesn't), so we do what they do and just print it
136+
137+
// Just print a Python-like stack trace; CPython does the same (see
138+
// 'weakrefobject.c: handle_callback')
139+
ExceptionUtils.printPythonLikeStackTrace(e);
140+
}
131141
}
132-
}
142+
} while (proceed());
133143
}
134144
}
135145

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/PythonContext.java

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
102102
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
103103
import com.oracle.truffle.api.ContextThreadLocal;
104+
import com.oracle.truffle.api.ThreadLocalAction;
104105
import com.oracle.truffle.api.Truffle;
105106
import com.oracle.truffle.api.TruffleFile;
106107
import com.oracle.truffle.api.TruffleLanguage;
@@ -1154,7 +1155,12 @@ private void joinThreads() {
11541155
// in the acquireGil function, which will be interrupted for these threads
11551156
disposeThread(thread);
11561157
for (int i = 0; i < 100 && thread.isAlive(); i++) {
1157-
thread.interrupt();
1158+
env.submitThreadLocal(new Thread[]{thread}, new ThreadLocalAction(true, false) {
1159+
@Override
1160+
protected void perform(ThreadLocalAction.Access access) {
1161+
throw new PythonThreadKillException();
1162+
}
1163+
});
11581164
thread.join(2);
11591165
}
11601166
if (thread.isAlive()) {
@@ -1229,6 +1235,16 @@ public PythonNativeWrapper getSingletonNativeWrapper(PythonAbstractObject obj) {
12291235
return null;
12301236
}
12311237

1238+
public void reacquireGilAfterStackOverflow() {
1239+
while (!ownsGil()) {
1240+
try {
1241+
acquireGil();
1242+
} catch (InterruptedException ignored) {
1243+
// just keep trying
1244+
}
1245+
}
1246+
}
1247+
12321248
/**
12331249
* Should not be called directly.
12341250
*
@@ -1263,25 +1279,10 @@ boolean tryAcquireGil() {
12631279
@TruffleBoundary
12641280
void acquireGil() throws InterruptedException {
12651281
assert !ownsGil() : dumpStackOnAssertionHelper("trying to acquire the GIL more than once");
1266-
try {
1267-
globalInterpreterLock.lockInterruptibly();
1268-
} catch (InterruptedException e) {
1269-
if (!ImageInfo.inImageBuildtimeCode() && getThreadState(getLanguage()).isShuttingDown()) {
1270-
// This is a thread being killed during normal context shutdown. This thread
1271-
// should exit now. This should usually only happen for daemon threads on
1272-
// context shutdown. This is the equivalent to the logic in pylifecycle.c and
1273-
// PyEval_RestoreThread which, on Python shutdown, will join non-daemon threads
1274-
// and then simply start destroying the thread states of remaining threads. If
1275-
// any remaining daemon thread then tries to acquire the GIL, it'll notice the
1276-
// shutdown is happening and exit.
1277-
throw new PythonThreadKillException();
1278-
} else {
1279-
// We are being interrupted through some non-internal means. Commonly this may be
1280-
// because Truffle wants to run some safepoint action. In this case, we need to
1281-
// rethrow the InterruptedException.
1282-
Thread.interrupted();
1283-
throw e;
1284-
}
1282+
boolean wasInterrupted = Thread.interrupted();
1283+
globalInterpreterLock.lockInterruptibly();
1284+
if (wasInterrupted) {
1285+
Thread.currentThread().interrupt();
12851286
}
12861287
}
12871288

0 commit comments

Comments
 (0)