JoegenUSTC opened a new pull request, #11846:
URL: https://github.com/apache/gravitino/pull/11846
### What changes were proposed in this pull request?
This PR fixes a race condition between `CatalogWrapper.close()` and
in-flight `Capability` method calls that causes `NoClassDefFoundError`
in catalog operations. It supersedes #11755.
**Root cause**
`Caffeine` cache eviction calls `CatalogWrapper.close()` asynchronously,
tearing down the `IsolatedClassLoader` while another thread may still hold
a `Capability` object loaded by that classloader. On the first call to a
switch-on-enum method such as `caseSensitiveOnName()`, the JVM lazily loads
a compiler-generated synthetic class (`HiveCatalogCapability$1`) via the
**defining classloader** — not the thread-context classloader. If that
classloader is already closed, the load fails with `NoClassDefFoundError`,
and the JVM **permanently caches the failure** for the lifetime of the
process.
**Why the previous approach (#11755) was insufficient**
PR #11755 wrapped the `Capability` object in a JDK dynamic proxy that
switched TCCL before each method call. However, switching the TCCL does
not fix the root issue: the synthetic class `$1` is loaded by the **defining
classloader** (the `IsolatedClassLoader`), not by TCCL. Once the classloader
is closed, no TCCL switch can recover it. Reviewer yuqi1129 verified this
empirically:
> *"SwitchCap (close loader, then call via withClassLoader/TCCL)" → FAILED*
Reviewer diqiu50 correctly identified the true fix:
> *"close() should wait for all in-flight operations before releasing the
> classloader"* and *"CatalogWrapper should expose a doWithCapabilityOps()
> method so callers receive safe values, not the plugin Capability instance
> itself."*
**Fix — Part 1: `ReadWriteLock` on `CatalogWrapper`**
- Add `ReentrantReadWriteLock` to `CatalogWrapper`.
- All `doWithXxxOps()` methods acquire the **read lock** before executing;
`close()` acquires the **write lock**, so it must wait for all in-flight
operations to complete before tearing down the classloader.
- `close()` is idempotent via `volatile boolean closed`.
**Fix — Part 2: `doWithCapabilityOps` prevents `Capability` escape**
- Add `doWithCapabilityOps(fn)` which holds the **read lock** for the
entire `Capability` lambda, ensuring no `Capability` method executes
against a closed classloader.
- Add `CapabilityHelpers.withCapability(ident, mgr, fn)` as the
public entry point.
- Migrate all `getCapability()` call sites in `NormalizeDispatcher`
classes, `OperationDispatcher.isManagedEntity()`,
`CatalogManager.isManagedStorageCatalog()`, and
`MetadataIdConverter.normalizeCaseSensitive()` to
`withCapability()` / `doWithCapabilityOps()`, so no `Capability`
object ever escapes the lock boundary.
### Why are the changes needed?
Without this fix, a catalog evicted from the Caffeine cache under load
can cause a permanent `NoClassDefFoundError` for all subsequent
operations against any catalog of the same type in the same JVM process.
The root issue (#11726) was reported as an unrecoverable
`HiveCatalogCapability$1` load failure.
### Does this PR introduce _any_ user-facing change?
No API or configuration changes. The behavior change is purely internal:
catalog operations now fail fast with `IllegalStateException` if the
catalog has already been closed, rather than crashing with a cryptic
classloader error.
### How was this patch tested?
- `TestCatalogWrapperConcurrency` (new): four targeted concurrent tests
covering (1) `close()` blocks until in-flight `doWithCapabilityOps()`
finish, (2) `close()` is idempotent, (3) ops after `close()` throw
`IllegalStateException`, (4) concurrent `close()` calls do not
double-close the classloader.
- Updated mock stubs in `TestTableHookDispatcher`,
`TestTopicHookDispatcher`, `TestFilesetHookDispatcher`,
`TestFunctionHookDispatcher`, `TestModelHookDispatcher`,
`TestSchemaHookDispatcher`, and `TestCatalogManager` to cover the new
`doWithCapabilityOps` path.
- All existing unit tests pass (`./gradlew :core:test :server-common:test
-PskipITs`).
- Full build passes (`./gradlew clean build -x test`).
Fix: #11726
--
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]
