SEZ9 commented on PR #12391: URL: https://github.com/apache/seatunnel/pull/12391#issuecomment-5975655877
Thanks @SeaSand1024 for calling out the classloader-restore consequence explicitly. **On F2:** agreed that an `Error` escaping the BlockingWorker fallback before `setContextClassLoader(oldClassLoader)` is not a merge blocker on its own; if the JVM is already in an OOM / `StackOverflowError` state, an unrestored context classloader is the least of the problems. A nested `try/finally` so the restore runs unconditionally is small and I would take it as polish, but if you would rather leave it, just say so. **What I want to resolve before merge** is how the narrowed `catch (Exception)` interacts with the close-once latch at `877056bf1`: - A lifecycle `close()` that throws an `Error` (realistically `NoClassDefFoundError` from a connector classloader, not OOM) now escapes the per-lifecycle loop, so every lifecycle after it is skipped. - Could you confirm when the `closed` latch is set relative to that loop? If it is already set when the `Error` escapes, the fallback's second `close()` becomes a no-op and nothing ever closes the remaining lifecycles. If so, F1 (idempotency) and F2 (not swallowing fatal errors) are individually right, but combined they lose the full-pass guarantee for exactly the unchecked-failure class this PR exists to handle. Two options, either is fine: 1. In the per-lifecycle loop, catch `Throwable`, keep iterating so the full pass completes, then rethrow the first failure, with an `Error` taking priority over any `Exception` so a fatal error is never downgraded to a log line. 2. Keep `catch (Exception)` in the loop but only set `closed` after the loop has completed the full pass, so an escaping `Error` leaves the latch unset and the fallback can still finish teardown. I lean to option 1 as the smaller behavioral surface. **F3:** please add one test that drives the `Error`-from-a-lifecycle case end to end (lifecycle A throws `NoClassDefFoundError`, assert lifecycle B still gets `close()` and the `Error` is what surfaces), plus one for the `super.close()` failure branch. **F1 / F5:** could you point me to the relevant hunks at `877056bf1` (the close-once latch and the null `allCycles` guard) so I can close them out? **F4** is fine once the Javadoc throws clauses match whichever option you pick above. <!-- streview-comment:1492 --> -- 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]
