DanielLeens commented on PR #12626: URL: https://github.com/apache/seatunnel/pull/12626#issuecomment-6538131399
Pushed the follow-up commit announced above: `cbd95115af88f1895022172f2df3476951128545` (previous head `125cbbad4d`). Mapping of @SEZ9's items to the new head (paths relative to `seatunnel-engine/seatunnel-engine-core/src/main/java/org/apache/seatunnel/engine/core/classloader/` unless stated): - **Issue 1 (forced `<clinit>` of `PowerOfTwoBufferPool`)** - `ReleasedClassLoaderReferenceCleaner.java:157` now resolves the pool through `ClassLoader.findLoadedClass` (`findLoadedClass` helper at `:368`, resolved once at `:106`/`:376`) instead of `Class.forName(name, false, released)`. A loader that never loaded the pool class is skipped, so nothing is defined into the dying loader and no pruner is started just to be stopped. `findLoadedClass` is protected, so it is reached reflectively; the shipped `config/jvm_options` already carry `--add-opens java.base/java.lang=ALL-UNNAMED`, and if the method is not accessible the step falls back to the previous resolve path with a one-time WARN (`:376-389`), preferring the extra work over bringing the leak back. Remaining corner: a class the job loaded but never initialized is still initialized by the static read; that only happens for classes the job itself loaded, not for loaders that merely had the driver on the jar path. Test: `Release dClassLoaderReferenceCleanerTest.leavesPoolClassUnloadedWhenReleasedLoaderNeverUsedIt` (asserts the pool class is neither defined into the released loader nor initialized, via a parent-first initialization counter); `keepsPrunerOfPoolDefinedByAncestorLoader` now loads the class through the released loader first so it still exercises the ownership guard rather than the new gate. - **Issue 2 (`clean()` not fully best-effort, call site before bookkeeping)** - every step catches `Throwable` (`:172`, `:248`, `:265`), and `DefaultClassLoaderService.releaseClassLoader` now runs the cleaner after the `classLoaderMap.isEmpty()` bookkeeping (`DefaultClassLoaderService.java:165-183`) inside its own `try/catch (Throwable)` (`cleanReleasedClassLoaderReferences`, `:186-197`), so the per-job maps are always consistent and `TaskExecutionService.recycleClassLoader` keeps iterating the remaining jar sets whatever the cleanup does. Test: `cleansRemainingRootsWhenOneRootThrowsAnError` (a pool fixture whose static initializer throws `OutOfMemoryError`, loaded but not initialized by the "job"; the Hadoop roots of the same loader must still be cleaned and `clean()` must not throw - this test is red against the previous catch clause). - **Issue 3 (reflective contract only checked against fixtures)** - added two contract tests in modules that have the real jars on their test class path and no `pom.xml` change: `seatunnel-connectors-v2/connector-mongodb/src/test/java/org/apache/seatunnel/connectors/seatunnel/mongodb/MongoBufferPoolPrunerContractTest.java` (static `DEFAULT` of the pool type and no-arg instance `disablePruning()` on `com.mongodb.internal.connection.PowerOfTwoBufferPool`, resolved without initialization) and `seatunnel-connectors-v2/connector-file/connector-file-base/src/test/java/org/apache/seatunnel/connectors/seatunnel/file/hadoop/HadoopClassLoaderRootsContractTest.java` (`WritableComparator.comparators` is a static `Map`, `getConf`/`setConf(Configuration)`, `Configuration.getClassLoader()`, and the four static `CodecPool` registries are a `Map` or expose `asMap()` returning one). A driver or Hadoop bump that changes any of these shapes now fails the unit-test job instead of silently reverting to the leak. - **Issue 4 (hard-coded internals without pointers)** - the class Javadoc (`ReleasedClassLoaderReferenceCleaner.java:69-80`) now records the verified versions (mongodb-driver-core 4.7.1 via `mongo.driver.version`, Hadoop 3.1.4/3.3.6 uber), names the two contract tests as the build-side guard, and states that a connector-owned SPI release hook is the intended longer-term home. **Not done:** the suggested one-line comments next to `mongo.driver.version` in `connector-mongodb/pom.xml` and the hadoop uber version property, because this PR deliberately touches no `pom.xml`; the contract tests in exactly those two modules are what a version bump will hit. If maintainers prefer the pom comments as well, I will add them in a separate commit. - **Issue 5 (registry iterated without a lock, `getConf`/`setConf` not atomic)** - the comparator loop runs under the registry's own monitor when the registry is not a `ConcurrentMap` (`:205-206`; Hadoop's `ConcurrentHashMap` needs no lock and a private monitor is used so nothing shared is locked), and the `Configuration` is re-read immediately before `setConf(null)` (`:232-234`), so a `Configuration` that a running job stored in between is kept. As noted in the review, the window can be narrowed but not closed from outside Hadoop, since `WritableComparator.get` writes without a lock. - **Issue 6 (WARN flood, no kill switch)** - `logStepFailure` (`:335-346`) logs the first failure of a given (step, exception class, message) signature at WARN with the stack trace and identical later failures at DEBUG. New operator switch `CLASSLOADER_SERVICE_SKIP_RELEASED_REFERENCE_CLEANUP` (`DefaultClassLoaderService.java:55-56`, read in the public constructor at `:64-73`, same style as `CLASSLOADER_SERVICE_SKIP_CHECK_JAR`) disables the cleaner; `SKIP_CHECK_JAR` is not documented in `docs/`, so no docs entry was added for the sibling switch. Test: `serviceReleasesWithoutCleaningWhenCleanerIsDisabled`. - **Issue 7 (`entrie(s)`)** - fixed at `:271`. Local verification was again limited to `./mvnw spotless:apply` / `spotless:check` on the three touched modules; compile and tests are left to the fork CI per the validation policy: https://github.com/DanielLeens/seatunnel/actions/runs/41162411362. The PR is still `MERGEABLE` against `dev`. -- 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]
