LuciferYang commented on code in PR #12146:
URL: https://github.com/apache/gravitino/pull/12146#discussion_r3642712686
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java:
##########
@@ -220,6 +220,13 @@ public void stop() throws IOException,
InterruptedException {
sleepUninterruptibly(500, TimeUnit.MILLISECONDS);
executor.shutdownNow();
+ // The HTTP port may be closed before GravitinoServer.main() finishes
shutting down the
+ // singleton GravitinoEnv. Wait for the server task to terminate so the
next embedded server
+ // cannot initialize the same environment while this shutdown is still in
progress.
+ if (!executor.awaitTermination(3, TimeUnit.MINUTES)) {
Review Comment:
The new `awaitTermination` check sits before `restClient.close()` and
`deleteDirectory(mockConfDir)`, so when it times out (RuntimeException) or the
wait is interrupted (InterruptedException), both cleanup steps are skipped and
the restClient and temp dir leak. The previous `if (started) throw` ran after
cleanup, so this is introduced by this PR. Moving the restClient/temp-dir
cleanup into a `finally` covers it; note `restClient.close()` needs its own
try/catch inside the finally, otherwise the IOException it may throw would
replace the in-flight RuntimeException/InterruptedException (`deleteDirectory`
already has a try/catch(ignore), so it's fine).
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -583,7 +583,7 @@ public void start() {
}
/** Shutdown the Gravitino environment. */
- public void shutdown() {
+ public synchronized void shutdown() {
Review Comment:
I think what actually fixes this issue here is `awaitTermination` in
`MiniGravitino.stop()`; the `synchronized` half doesn't match what the
description claims. `synchronized` only prevents init/shutdown from running
concurrently — it doesn't stop an old shutdown that has fully completed from
acting on the next generation's fields. `shutdown()` reads the singleton's
current `catalogManager` at execution time (and never nulls it), so if B's init
takes the lock and sets the field first, then A's shutdown takes the lock
afterwards, `catalogManager.close()` still closes B's manager. `start()` also
isn't synchronized and so isn't mutually exclusive with `shutdown()`, so this
set of locks doesn't really establish a "one lifecycle op at a time" invariant.
The lock is harmless in a single-JVM production run, so no code change is
needed, but I'd reword the "synchronized prevents an unfinished shutdown from
closing a new server's components" line to reflect reality: the real
serialization
comes from `awaitTermination`, and `synchronized` is just extra defense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
