LuciferYang commented on PR #12124: URL: https://github.com/apache/gravitino/pull/12124#issuecomment-5583860776
@yuqi1129 Thanks for verifying it rather than taking the issue's word for it. The ordering call was right, both fixes are in, and there is one suggestion I decided against. **Ordering.** The MySQL shutdown now runs above `stopThreadsAndClearThreadLocalVariables`, with the reason inline: the driver's factory sets the cleanup thread's context ClassLoader to the loader that defined the driver, which is exactly what the generic sweep matches on. The driver now leaves through its own API and the sweep finds no thread to interrupt. **Sweep scope: kept broad, allowlist declined.** What bounds this sweep is not the thread set, it is the value check. An entry is cleared only when `value.getClass().getClassLoader() == targetClassLoader`, so visiting a ForkJoinPool worker or a Hadoop daemon touches nothing unless that thread is holding a value loaded by the catalog being dropped. The allowlist would put the thread set back in charge of correctness, and every name nobody thought of becomes a silent miss with the loader still pinned and no signal anywhere. That is the failure mode #8252 shipped, and it took an issue to find. A `RUNNABLE` check has a different problem: the state can change between the check and the write, so it buys the appearance of safety rather than safety. The residual risk you named is real and I am not claiming otherwise. A value belonging to the departing catalog can be nulled while a thread is still using it. That work is against a catalog being dropped and fails either way, and the value check means no other ClassLoader's entries are ever in reach, but the window exists. The comment on `clearThreadLocalMap` now says that instead of implying the sweep is safe because of who it skips. **Class javadoc** records the vendor pattern for whoever adds the next one: guard with `isOwnedByClassLoader`, prefer the library's own shutdown API over interrupting its thread, and run the step before the generic sweep. **Tests.** Two, both mutation checked. The first is the reclaim test for the MySQL half, with your control group, because you are right that a bare `WeakReference` plus `System.gc()` proves little on its own. It asserts the control loader is collectable, then loads the driver in a child loader, drops every strong reference and asserts the loader is *not* collected, recovers it through `ref.get()`, runs the shutdown, and asserts it becomes collectable. Deleting the shutdown call fails it. The second drives the step sequence, which nothing did before. `closeClassLoaderResource` returns immediately when `GRAVITINO_TEST` is set and the root build sets that on every test task, so clearing it in-process would mean reflecting into `ProcessEnvironment` and mutating state shared with every other test in the JVM. Instead the step list is now a package-private `runCleanupSteps` and the test drives that. Removing the MySQL step from the sequence fails it. One thing I will not overclaim: that second test does not catch the ordering bug. Under the old order the sweep interrupts the thread and `uncheckedShutdown()` still clears the map afterwards, so the end state matches, exactly as you expected. What it buys is that the sequence is now exercised at all, so a future reordering that does change the outcome fails instead of passing quietly. The ordering fix rests on the mechanism you traced in the bytecode, not on a failing test. **Two points for the description**, both going in. `com.mysql.*` is a shared prefix: `isSharedClass` returns true for it, so `IsolatedClassLoader` asks `baseClassLoader` first and only falls through to `super.loadClass` when the app loader misses. A driver on the server classpath is therefore app-loaded, nothing is pinned to the isolated loader, and this step correctly skips through the `isOwnedByClassLoader` guard. The documented layout, driver in `catalogs/jdbc-mysql/libs/` only, is the one that takes the fall-through and leaks. So whether this fix does anything depends on where the operator put the jar, and the guard is what makes both layouts safe. The `Gravitino-webserver-` restriction arrived with the class in #8252, so this widens a fix that was incomplete from the start rather than reversing a decision someone made after an incident. Worth having in the description so the next reader does not have to run blame to find that out. -- 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]
