Skip to content

Commit 7b63dd0

Browse files
jchaloujchalou
committed
[GR-42539] Don't close engine on VM shutdown.
PullRequest: graal/13736
2 parents 8ef2a17 + 5ec2387 commit 7b63dd0

File tree

13 files changed

+114
-90
lines changed

13 files changed

+114
-90
lines changed

‎tools/src/com.oracle.truffle.tools.coverage/src/com/oracle/truffle/tools/coverage/impl/CoverageInstrument.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import java.io.PrintStream;
3333
import java.util.function.Function;
3434

35-
import com.oracle.truffle.api.exception.AbstractTruffleException;
3635
import org.graalvm.options.OptionCategory;
3736
import org.graalvm.options.OptionDescriptors;
3837
import org.graalvm.options.OptionKey;
@@ -43,6 +42,7 @@
4342
import org.graalvm.polyglot.Instrument;
4443

4544
import com.oracle.truffle.api.Option;
45+
import com.oracle.truffle.api.exception.AbstractTruffleException;
4646
import com.oracle.truffle.api.instrumentation.SourceSectionFilter;
4747
import com.oracle.truffle.api.instrumentation.TruffleInstrument;
4848
import com.oracle.truffle.tools.coverage.CoverageTracker;
@@ -175,7 +175,7 @@ protected void onCreate(Env env) {
175175
}
176176

177177
@Override
178-
protected void onDispose(Env env) {
178+
protected void onFinalize(Env env) {
179179
if (enabled) {
180180
SourceCoverage[] coverage = tracker.getCoverage();
181181
final OptionValues options = env.getOptions();
@@ -195,6 +195,12 @@ protected void onDispose(Env env) {
195195
new LCOVPrinter(out, coverage, strictLines).print();
196196
break;
197197
}
198+
}
199+
}
200+
201+
@Override
202+
protected void onDispose(Env env) {
203+
if (enabled) {
198204
tracker.close();
199205
}
200206
}

‎tools/src/com.oracle.truffle.tools.profiler/src/com/oracle/truffle/tools/profiler/impl/CPUSamplerInstrument.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,16 @@ protected OptionDescriptors getOptionDescriptors() {
138138
* @since 0.30
139139
*/
140140
@Override
141-
protected void onDispose(Env env) {
141+
protected void onFinalize(Env env) {
142142
OptionValues options = env.getOptions();
143143
CPUSamplerCLI.EnableOptionData enableOptionData = options.get(CPUSamplerCLI.ENABLED);
144144
if (enableOptionData.enabled) {
145145
CPUSamplerCLI.handleOutput(env, sampler);
146146
}
147+
}
148+
149+
@Override
150+
protected void onDispose(Env env) {
147151
sampler.close();
148152
}
149153
}

‎tools/src/com.oracle.truffle.tools.profiler/src/com/oracle/truffle/tools/profiler/impl/CPUTracerInstrument.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,15 @@ protected OptionDescriptors getOptionDescriptors() {
143143
* @since 0.30
144144
*/
145145
@Override
146-
protected void onDispose(Env env) {
146+
protected void onFinalize(Env env) {
147147
if (enabled) {
148148
CPUTracerCLI.handleOutput(env, tracer);
149+
}
150+
}
151+
152+
@Override
153+
protected void onDispose(Env env) {
154+
if (enabled) {
149155
tracer.close();
150156
}
151157
}

‎tools/src/com.oracle.truffle.tools.profiler/src/com/oracle/truffle/tools/profiler/impl/MemoryTracerInstrument.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,14 @@ protected OptionDescriptors getOptionDescriptors() {
140140
* @since 0.30
141141
*/
142142
@Override
143-
protected void onDispose(Env env) {
143+
protected void onFinalize(Env env) {
144144
if (env.getOptions().get(MemoryTracerCLI.ENABLED)) {
145145
MemoryTracerCLI.handleOutput(env, tracer);
146146
}
147+
}
148+
149+
@Override
150+
protected void onDispose(Env env) {
147151
tracer.close();
148152
}
149153
}

‎truffle/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ This changelog summarizes major changes between Truffle versions relevant to lan
4343
* GR-43903 Usages of `@Specialization(assumptions=...)` that reach a `@Fallback` specialization now produce a suppressable warning. In most situations, such specializations should be migrated to use a regular guard instead. For example, instead of using `@Specialization(assumptions = "assumption")` you might need to be using `@Specialization(guards = "assumption.isValid()")`.
4444
* GR-43903 Added `@Idempotent` and `@NonIdempotent` DSL annotations useful for DSL guard optimizations. Guards that only bind idempotent methods and no dynamic values can always be assumed `true` after they were `true` once on the slow-path. The generated code leverages this information and asserts instead of executes the guard on the fast-path. The DSL now emits warnings with for all guards where specifying the annotations may be beneficial. Note that all guards that do not bind dynamic values are assumed idempotent by default for compatibility reasons.
4545
* GR-43663 Added RootNode#computeSize as a way for languages to specify an approximate size of a RootNode when number of AST nodes cannot be used (e.g. for bytecode interpreters).
46+
* GR-42539 (change of behavior) Unclosed polyglot engines are no longer closed automatically on VM shutdown. They just die with the VM. As a result, `TruffleInstrument#onDispose` is not called for active instruments on unclosed engines in the event of VM shutdown. In case an instrument is supposed to do some specific action before its disposal, e.g. print some kind of summary, it should be done in `TruffleInstrument#onFinalize`.
4647

4748
## Version 22.3.0
4849

‎truffle/src/com.oracle.truffle.api.instrumentation/src/com/oracle/truffle/api/instrumentation/TruffleInstrument.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,12 @@ protected TruffleInstrument() {
173173
* languages are going to be disposed, possibly because the underlying
174174
* {@linkplain org.graalvm.polyglot.Engine engine} is going to be closed. This method is called
175175
* before {@link #onDispose(Env)} and the instrument must remain usable after finalization. The
176-
* instrument can prepare for disposal while still having other instruments not disposed yet.
177-
*
176+
* instrument can prepare for disposal while still having other instruments not disposed yet. In
177+
* the event of VM shutdown, {@link #onDispose(Env)} for active instruments on unclosed
178+
* {@link org.graalvm.polyglot.Engine engines} is not called, and so in case the instrument is
179+
* supposed to do some specific action before its disposal, e.g. print some kind of summary, it
180+
* should be done in this method.
181+
*
178182
* @param env environment information for the instrument
179183
* @since 19.0
180184
*/
@@ -186,7 +190,9 @@ protected void onFinalize(Env env) {
186190
* Invoked once on an {@linkplain TruffleInstrument instance} when it becomes disabled, possibly
187191
* because the underlying {@linkplain org.graalvm.polyglot.Engine engine} has been closed. A
188192
* disposed instance is no longer usable. If the instrument is re-enabled, the engine will
189-
* create a new instance.
193+
* create a new instance. In the event of VM shutdown, this method is not called for active
194+
* instruments on unclosed {@link org.graalvm.polyglot.Engine engines}. The unclosed engines are
195+
* not closed automatically on VM shutdown, they just die with the VM.
190196
*
191197
* @param env environment information for the instrument
192198
* @since 0.12

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1722,7 +1722,7 @@ public void closeContext(Object impl, boolean force, boolean resourceExhaused, S
17221722
@Override
17231723
public void closeEngine(Object polyglotEngine, boolean force) {
17241724
PolyglotEngineImpl engine = (PolyglotEngineImpl) polyglotEngine;
1725-
engine.ensureClosed(force, false, false);
1725+
engine.ensureClosed(force, false);
17261726
}
17271727

17281728
@Override

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
import org.graalvm.polyglot.PolyglotException;
8686
import org.graalvm.polyglot.Value;
8787
import org.graalvm.polyglot.impl.AbstractPolyglotImpl.AbstractHostLanguageService;
88+
import org.graalvm.polyglot.io.IOAccess;
8889

8990
import com.oracle.truffle.api.CallTarget;
9091
import com.oracle.truffle.api.CompilerAsserts;
@@ -121,7 +122,6 @@
121122
import com.oracle.truffle.polyglot.PolyglotLocals.LocalLocation;
122123
import com.oracle.truffle.polyglot.PolyglotThreadLocalActions.HandshakeConfig;
123124
import com.oracle.truffle.polyglot.SystemThread.LanguageSystemThread;
124-
import org.graalvm.polyglot.io.IOAccess;
125125

126126
final class PolyglotContextImpl implements com.oracle.truffle.polyglot.PolyglotImpl.VMObject {
127127

@@ -1642,7 +1642,7 @@ void closeAndMaybeWait(boolean force, List<Future<Void>> futures) {
16421642
engine.polyglotHostService.notifyContextClosed(this, force, invalidResourceLimit, invalidMessage);
16431643
}
16441644
if (engine.boundEngine && parent == null) {
1645-
engine.ensureClosed(force, false, true);
1645+
engine.ensureClosed(force, true);
16461646
}
16471647
}
16481648

‎truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotEngineDispatch.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public Instrument requirePublicInstrument(Object oreceiver, String id) {
114114
public void close(Object oreceiver, Object apiObject, boolean cancelIfExecuting) {
115115
PolyglotEngineImpl receiver = (PolyglotEngineImpl) oreceiver;
116116
try {
117-
receiver.ensureClosed(cancelIfExecuting, false, false);
117+
receiver.ensureClosed(cancelIfExecuting, false);
118118
} catch (Throwable t) {
119119
throw PolyglotImpl.guestToHostException(receiver, t);
120120
}

‎truffle/src/com.oracle.truffle.polyglot/src/com/oracle/truffle/polyglot/PolyglotEngineImpl.java‎

Lines changed: 62 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,6 @@
127127
import com.oracle.truffle.api.nodes.Node;
128128
import com.oracle.truffle.api.nodes.RootNode;
129129
import com.oracle.truffle.api.source.SourceSection;
130-
import com.oracle.truffle.polyglot.SystemThread.InstrumentSystemThread;
131130
import com.oracle.truffle.polyglot.PolyglotContextConfig.FileSystemConfig;
132131
import com.oracle.truffle.polyglot.PolyglotContextConfig.PreinitConfig;
133132
import com.oracle.truffle.polyglot.PolyglotContextImpl.ContextWeakReference;
@@ -137,6 +136,7 @@
137136
import com.oracle.truffle.polyglot.PolyglotLocals.LocalLocation;
138137
import com.oracle.truffle.polyglot.PolyglotLoggers.EngineLoggerProvider;
139138
import com.oracle.truffle.polyglot.PolyglotLoggers.LoggerCache;
139+
import com.oracle.truffle.polyglot.SystemThread.InstrumentSystemThread;
140140

141141
final class PolyglotEngineImpl implements com.oracle.truffle.polyglot.PolyglotImpl.VMObject {
142142

@@ -1177,7 +1177,7 @@ <T extends TruffleLanguage<?>> PolyglotLanguage getLanguage(Class<T> languageCla
11771177
return foundLanguage;
11781178
}
11791179

1180-
void ensureClosed(boolean force, boolean inShutdownHook, boolean initiatedByContext) {
1180+
void ensureClosed(boolean force, boolean initiatedByContext) {
11811181
synchronized (this.lock) {
11821182
Thread currentThread = Thread.currentThread();
11831183
boolean interrupted = false;
@@ -1200,66 +1200,49 @@ void ensureClosed(boolean force, boolean inShutdownHook, boolean initiatedByCont
12001200
* Check ahead of time for open contexts to fail early and avoid closing only some
12011201
* contexts.
12021202
*/
1203-
if (!inShutdownHook) {
1204-
if (!force) {
1205-
for (PolyglotContextImpl context : localContexts) {
1206-
assert !Thread.holdsLock(context);
1207-
synchronized (context) {
1208-
if (context.hasActiveOtherThread(false) && !context.state.isClosing()) {
1209-
throw PolyglotEngineException.illegalState(String.format("One of the context instances is currently executing. " +
1210-
"Set cancelIfExecuting to true to stop the execution on this thread."));
1211-
}
1203+
if (!force) {
1204+
for (PolyglotContextImpl context : localContexts) {
1205+
assert !Thread.holdsLock(context);
1206+
synchronized (context) {
1207+
if (context.hasActiveOtherThread(false) && !context.state.isClosing()) {
1208+
throw PolyglotEngineException.illegalState(String.format("One of the context instances is currently executing. " +
1209+
"Set cancelIfExecuting to true to stop the execution on this thread."));
12121210
}
12131211
}
12141212
}
1215-
if (!initiatedByContext) {
1216-
/*
1217-
* context.cancel and context.closeAndMaybeWait close the engine if it is bound
1218-
* to the context, so if we called these methods here, it might lead to
1219-
* StackOverflowError.
1220-
*/
1221-
for (PolyglotContextImpl context : localContexts) {
1222-
assert !Thread.holdsLock(context);
1223-
assert context.parent == null;
1224-
if (force) {
1225-
context.cancel(false, null);
1226-
} else {
1227-
context.closeAndMaybeWait(false, null);
1228-
}
1213+
}
1214+
if (!initiatedByContext) {
1215+
/*
1216+
* context.cancel and context.closeAndMaybeWait close the engine if it is bound to
1217+
* the context, so if we called these methods here, it might lead to
1218+
* StackOverflowError.
1219+
*/
1220+
for (PolyglotContextImpl context : localContexts) {
1221+
assert !Thread.holdsLock(context);
1222+
assert context.parent == null;
1223+
if (force) {
1224+
context.cancel(false, null);
1225+
} else {
1226+
context.closeAndMaybeWait(false, null);
12291227
}
12301228
}
12311229
}
12321230

1233-
// don't commit changes to contexts if still running
1234-
if (!inShutdownHook) {
1235-
contexts.clear();
1231+
contexts.clear();
12361232

1237-
if (RUNTIME.onEngineClosing(this.runtimeData)) {
1238-
return;
1239-
}
1233+
if (RUNTIME.onEngineClosing(this.runtimeData)) {
1234+
return;
12401235
}
12411236
closingThread = currentThread;
12421237
}
12431238

12441239
// instruments should be shut-down even if they are currently still executed
12451240
// we want to see instrument output if the process is quit while executing.
12461241
for (PolyglotInstrument instrumentImpl : idToInstrument.values()) {
1247-
try {
1248-
instrumentImpl.notifyClosing();
1249-
} catch (Throwable e) {
1250-
if (!inShutdownHook) {
1251-
throw e;
1252-
}
1253-
}
1242+
instrumentImpl.ensureFinalized();
12541243
}
12551244
for (PolyglotInstrument instrumentImpl : idToInstrument.values()) {
1256-
try {
1257-
instrumentImpl.ensureClosed();
1258-
} catch (Throwable e) {
1259-
if (!inShutdownHook) {
1260-
throw e;
1261-
}
1262-
}
1245+
instrumentImpl.ensureClosed();
12631246
}
12641247

12651248
synchronized (this.lock) {
@@ -1284,27 +1267,22 @@ void ensureClosed(boolean force, boolean inShutdownHook, boolean initiatedByCont
12841267
getEngineLogger().log(Level.INFO, String.format("Specialization histogram: %n%s", logMessage.toString()));
12851268
}
12861269

1287-
if (!inShutdownHook) {
1288-
RUNTIME.onEngineClosed(this.runtimeData);
1270+
RUNTIME.onEngineClosed(this.runtimeData);
12891271

1290-
Object loggers = getEngineLoggers();
1291-
if (loggers != null) {
1292-
LANGUAGE.closeEngineLoggers(loggers);
1293-
}
1294-
if (logHandler != null) {
1295-
logHandler.close();
1296-
}
1272+
Object loggers = getEngineLoggers();
1273+
if (loggers != null) {
1274+
LANGUAGE.closeEngineLoggers(loggers);
1275+
}
1276+
if (logHandler != null) {
1277+
logHandler.close();
1278+
}
12971279

1298-
polyglotHostService.notifyEngineClosed(this, force);
1280+
polyglotHostService.notifyEngineClosed(this, force);
12991281

1300-
if (runtimeData != null) {
1301-
EngineAccessor.RUNTIME.flushCompileQueue(runtimeData);
1302-
}
1303-
getAPIAccess().engineClosed(api);
1304-
} else if (logHandler != null) {
1305-
// called from shutdown hook, at least flush the logging handler
1306-
logHandler.flush();
1282+
if (runtimeData != null) {
1283+
EngineAccessor.RUNTIME.flushCompileQueue(runtimeData);
13071284
}
1285+
getAPIAccess().engineClosed(api);
13081286
}
13091287
}
13101288

@@ -1822,7 +1800,7 @@ public PolyglotContextImpl createContext(OutputStream configOut, OutputStream co
18221800
if (contextAddedToEngine) {
18231801
disposeContext(context);
18241802
if (boundEngine) {
1825-
ensureClosed(false, false, false);
1803+
ensureClosed(false, false);
18261804
}
18271805
}
18281806
throw t;
@@ -1929,7 +1907,7 @@ private PolyglotContextImpl loadPreinitializedContext(PolyglotContextConfig conf
19291907
// If the patching fails we have to perform a silent engine close without
19301908
// notifying the new polyglotHostService.
19311909
polyglotHostService = new DefaultPolyglotHostService(impl);
1932-
ensureClosed(true, false, false);
1910+
ensureClosed(true, false);
19331911
synchronized (engine.lock) {
19341912
context = new PolyglotContextImpl(engine, config);
19351913
engine.addContext(context);
@@ -2211,7 +2189,25 @@ void onVMShutdown() {
22112189
log.println();
22122190
createdLocation.printStackTrace();
22132191
}
2214-
ensureClosed(false, true, false);
2192+
/*
2193+
* In the event of VM shutdown, the engine is not closed and active instruments on it are
2194+
* not disposed. Without closing the contexts on the engine, which is not possible during
2195+
* shutdown, because it could delay or block the shutdown, closing the engine could lead to
2196+
* unexpected errors. Therefore, we let the engine die with the VM and just notify the
2197+
* instruments by calling TruffleInstrument#onFinalize.
2198+
*/
2199+
for (PolyglotInstrument instrumentImpl : idToInstrument.values()) {
2200+
try {
2201+
instrumentImpl.ensureFinalized();
2202+
} catch (Throwable e) {
2203+
getEngineLogger().log(Level.WARNING, "Instrument " + instrumentImpl.getName() + " threw an exception during onFinalize.", e);
2204+
}
2205+
}
2206+
synchronized (this.lock) {
2207+
if (logHandler != null) {
2208+
logHandler.flush();
2209+
}
2210+
}
22152211
}
22162212

22172213
void addSystemThread(InstrumentSystemThread thread) {
@@ -2265,7 +2261,7 @@ static void resetFallbackEngine() {
22652261
}
22662262
}
22672263
if (engineToClose != null) {
2268-
engineToClose.ensureClosed(false, false, false);
2264+
engineToClose.ensureClosed(false, false);
22692265
}
22702266
}
22712267

0 commit comments

Comments
 (0)