JoegenUSTC commented on PR #11755:
URL: https://github.com/apache/gravitino/pull/11755#issuecomment-4795995313
> @yuqi1129 @JoegenUSTC I think the real issue is the race condition between
CatalogWrapper.close() and concurrent operations. close() should ensure no
other threads are still using the catalog before closing the `classloader`. We
need to fixed this issue
@diqiu50 @yuqi1129 — Agreed. The race condition between
`CatalogWrapper.close()` and concurrent operations is the correct diagnosis.
Here is my plan for a new PR targeting #11726:
**Part 1 — `ReadWriteLock` on `CatalogWrapper`** (~50 lines):
- All `doWithXxxOps()` methods acquire the read lock for the entire
operation, with an early `closed` guard.
- `close()` acquires the write lock and waits for all in-flight operations
to complete before closing the `IsolatedClassLoader`.
**Part 2 — Replace `capabilities()` with `doWithCapabilityOps(fn)`**
(per @diqiu50's original suggestion):
- The current `capabilities()` returns a live `Capability` object that
escapes the lock boundary. After the read lock is released, a concurrent
`close()` can shut the classloader before the caller invokes any
`Capability` method — at which point the lazy `$1` synthetic class
loading fails permanently.
- `doWithCapabilityOps(fn)` holds the **read lock from Part 1** for the
entire lambda, so the classloader cannot be closed while `$1` is being
loaded.
- Part 2 without Part 1's read lock does not eliminate the race —
`withClassLoader()` alone cannot prevent a concurrent `close()` from
shutting the classloader mid-execution, as @yuqi1129's test confirmed:
`SwitchCap` still fails even when called via `withClassLoader` after
the loader is closed.
- This requires refactoring ~40 `getCapability()` call sites across the 9
`NormalizeDispatcher` classes (~300 lines across multiple files).
**These two parts are interdependent**: Part 1 without Part 2 leaves the
`capabilities()` escape path unprotected. Part 2 without Part 1's read
lock does not prevent a concurrent `close()`. Only together do they fully
eliminate the race condition.
**On PR scope:** Given the size difference, would you prefer both parts in
a single PR, or Part 1 as a standalone first with Part 2 as a follow-up?
I'll close #11755 once the new PR is up.
--
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]
