[dengliming]

Fixes #6729

ExtensionLoader instances are now cached by both the SPI interface and the requesting ClassLoader. A plugin classloader therefore receives its own loader and its META-INF/shenyu resources are enumerated instead of reusing the application loader.

The regression test supplies an extension resource through a separate URLClassLoader and verifies that the plugin extension is discovered. All existing ExtensionLoader tests also pass.

Test:

• mvn -pl shenyu-spi -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=ExtensionLoaderClassLoaderTest,ExtensionLoaderTest -DfailIfNoTests=false test

[Copilot]

🟡 Changes recommended

Recursive cache updates, class-loader retention, and incompatibility with the production plugin loader remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates SPI loader caching to isolate extensions by interface and class loader.

Changes:

• Adds class-loader-aware cache keys.
• Adds regression coverage for extension resource discovery.

File summaries

File	Description
ExtensionLoader.java	Keys cached loaders by SPI class and class loader.
ExtensionLoaderClassLoaderTest.java	Tests discovery through a separate URL class loader.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Aias00]

Approved as PMC (Aias00). Small, well-scoped fix with regression tests; green CI, mergeable. Reviewed the diff.

[Aias00]

The direction of this change is right: keying the loader cache by (extensionClass, classLoader) and evicting on plugin-classloader close is exactly what was missing. But pr_build is currently red and the failure is caused by this change, so it needs to be fixed before this can land.

CI root cause (pr_build, 8m25s)

[shenyu-registry-core] Tests run: 8, Failures: 1, Errors: 4
<<< FAILURE! - org.apache.shenyu.registry.core.ShenyuInstanceRegisterRepositoryFactoryTest

oneProviderIsLoadedAndCached:83                        » IllegalArgument
multipleExternalProvidersKeepTheirSpiKeys:92           » IllegalArgument
initializationAndRenewalPreserveCacheContract:125      » IllegalArgument
uninitializedCachedInstanceKeepsExistingBehavior:141   » IllegalArgument
failedInitializationIsNotCachedAndFailedRenewalKeepsPreviousInstance:151
    expected: <java.lang.IllegalStateException> but was: <java.lang.IllegalArgumentException>

java.lang.IllegalArgumentException: No registry implementation found for registerType 'external'.
    Add the matching shenyu-registry-external dependency (or an external registry SPI provider)
    to the runtime classpath. Available types: []

ShenyuInstanceRegisterRepositoryFactoryTest is pre-existing on master and is not touched by this PR, but it was written against the exact defect this PR removes:

1. useProviders() writes a temporary META-INF/shenyu/<ShenyuInstanceRegisterRepository FQN> and creates a URLClassLoader over it, then calls ExtensionLoader.getExtensionLoader(ShenyuInstanceRegisterRepository.class, classLoader) to warm the cache.
2. ShenyuInstanceRegisterRepositoryFactory#loadRepository then calls the single-arg getExtensionLoader(clazz), which resolves to ExtensionLoader.class.getClassLoader().
3. On master the cache was keyed by clazz only, and the old code did LOADERS.putIfAbsent(clazz, new ExtensionLoader<>(clazz, cl)); return LOADERS.get(clazz); — so the factory silently received the loader the test had just warmed with its own classloader. The test passed because of the cache bleed.
4. With the new LoaderKey(clazz, classLoader) the bleed is gone, the factory gets a loader bound to the default classloader, shenyu-registry-core has no src/test/resources and no registry implementation module on its test classpath, so Available types: [] and every provider lookup fails.

So the implementation is behaving correctly — the test has to be updated in the same PR.

Required

• Update ShenyuInstanceRegisterRepositoryFactoryTest so it does not rely on the cache bleed. Making the classloader explicit on the factory entry point (e.g. loadRepository(registerType, classLoader) / an overload used by tests) matches the isolation semantics this PR introduces; writing the SPI file for the default classloader would keep testing the wrong thing.

• clearCaches() is now a no-op for the loader cache:

  ((Map<?, ?>) loaders.get(null)).remove(ShenyuInstanceRegisterRepository.class);

  LOADERS is a Map<LoaderKey, ExtensionLoader<?>> after this change, so removing by Class never matches anything and state leaks across test methods. Please also add a targeted eviction entry point next to removeExtensionLoaders(ClassLoader) (e.g. removeExtensionLoader(Class<?>, ClassLoader)) and use it here, instead of reaching into the private field.

Non-blocking review notes

• removeExtensionLoaders drops the LOADERS reference only. If plugin code still holds the ExtensionLoader instance, cachedClasses / cachedInstances / joinInstances keep strong references to plugin classes and Metaspace is still pinned. Worth documenting what close() guarantees, or clearing those maps too when the loader is evicted.
• findResource builds a new anonymous URLStreamHandler on every lookup. URL.equals / URL.hashCode on a custom protocol delegate to the handler (URLStreamHandler.equals inspects the host), which is both allocation-heavy and a surprising equality contract if a memory: URL ever lands in a Set/Map. A single static handler instance would be safer.
• loadDirectory iterates classLoader.getResources(fileName), i.e. parent and plugin resources are merged, which is why findResources is needed here — good. But when the platform and a plugin declare the same SPI key, loadClass is last-writer-wins and the ordering is parent-first, so a plugin cannot override a platform key. If that is intentional, a test pinning it down would help; if not, it is worth deciding now.
• The two build (shenyu-integrated-test-k8s-ingress-*) jobs also failed. They look unrelated to this change (container/network setup) — worth a re-run to separate flake from a real regression.

[Aias00]

Re-review after 14059859

All three points from the previous round are addressed, and the registry test is fixed by making the loader explicit rather than by restoring the accidental coupling.

Verified

1. The shenyu-registry-core failure is fixed at the root, not papered over

The previous round's failure was ShenyuInstanceRegisterRepositoryFactoryTest reporting Available types: [] for every case. The root cause was that the factory called the single-argument getExtensionLoader(clazz), which resolves to ExtensionLoader.class.getClassLoader(), and the old LOADERS implementation happened to return the loader the test had just warmed for its own URLClassLoader. Keying by (clazz, classLoader) removed that accidental sharing, and the test then correctly saw an empty registry SPI set.

The fix adds package-private loader overloads and switches every test call to pass its own loader:

static ShenyuInstanceRegisterRepository newInstance(final String registerType, final ClassLoader classLoader)
static ShenyuInstanceRegisterRepository loadRepository(final String registerType, final ClassLoader classLoader)
static ShenyuInstanceRegisterRepository newAndInitInstance(final RegisterConfig config, final ClassLoader classLoader)
static ShenyuInstanceRegisterRepository reNewAndInitInstance(final RegisterConfig config, final ClassLoader classLoader)

The public entry points delegate with ExtensionLoader.class.getClassLoader(), so production behaviour is unchanged and the tests no longer depend on cross-test cache bleed. The useProviders warm-up call was removed accordingly — that line only existed to create the coupling being removed.

2. clearCaches() is no longer a no-op

if (Objects.nonNull(classLoader)) {
    ExtensionLoader.removeExtensionLoader(ShenyuInstanceRegisterRepository.class, classLoader);
}

This replaces ((Map) loaders.get(null)).remove(...), which could never match anything once the key became a LoaderKey — the map was keyed by LoaderKey, and get(null) returned nothing. Using the new targeted removeExtensionLoader(clazz, classLoader) is the right primitive, and it avoids the broad removeExtensionLoaders(classLoader) sweep that would have evicted unrelated extension types in the same loader.

3. The anonymous URLStreamHandler is gone

loadDirectory now uses the standard path:

Enumeration<URL> urls = Objects.nonNull(this.classLoader) ? classLoader.getResources(fileName)
        : ClassLoader.getSystemResources(fileName);

and loadResources consumes them with url.openStream(). My earlier concern was that findResource minted a fresh anonymous URLStreamHandler per call, so a custom-scheme URL's equals/hashCode would dispatch into a handler instance with no identity relationship to the original — risky if those URLs were ever used as map keys or compared. That code is gone, so the concern is moot.

Two new tests worth calling out

externalProvidersDoNotLeakIntoDefaultLoader pins the exact regression this PR exists to fix:

assertEquals(TestRepository.class, ...loadRepository("external", classLoader).getClass());
assertThrows(IllegalArgumentException.class, () -> ...loadRepository("external"));

The first line proves the test loader can see its own SPI file; the second proves the default loader cannot see it. That is the isolation contract stated as an executable assertion, which is much stronger than the previous implicit dependency.

shouldEvictOnlyRequestedExtensionTypeAndClassLoader verifies eviction granularity: after removing (JdbcSPI, pluginClassLoader), a fresh loader is returned for that pair, while (JdbcSPI, applicationClassLoader) and (ExtensionFactory, pluginClassLoader) keep their original instances. This is the behaviour clearCaches() relies on.

Supporting checks

• LoaderKey.equals compares both fields with == and hashCode uses System.identityHashCode for both, so the contract is consistent and removeExtensionLoader finds the entry that getExtensionLoader inserted.
• LOADERS is a ConcurrentHashMap, so the putIfAbsent-then-return-previous pattern is correct.
• loadClass uses Class.forName(classPath, true, this.classLoader), so the class is genuinely defined by the requested loader — without this the whole isolation would be cosmetic.
• Duplicate SPI names for the same key still raise IllegalStateException rather than silently overwriting, which is the pre-existing behaviour.
• The javadoc on both removal methods now states that previously returned loaders and extensions must be released by the caller. That documents the limitation I raised last round instead of hiding it.

Non-blocking

REPOSITORY_MAP is still keyed by registerType alone, so newInstance("external", loaderA) followed by newInstance("external", loaderB) returns the loaderA instance from cache. The tests are unaffected because clearCaches() empties the map between cases, and production only ever uses one loader — but it means the factory's cache is not as isolated as the loader cache now is. If hot-reload ever calls the loader overloads with distinct loaders, this becomes a real leak. Worth a comment on the field at minimum.

CI

All 11 checks are QUEUED for bb0efb33, so I cannot confirm pr_build is green yet — that is the run that previously caught the registry failure, so it matters here. Approving on the code; I will merge once the run finishes.

Heads-up for that run: the branch merged master after #7903, which broke shenyu-integrated-test-http (CryptorResponsePluginTest.testWhenDoesntExistFiled, Expected: is "10001" but: was null). I am fixing that on master separately. If your build (shenyu-integrated-test-http) fails on that specific test, it is unrelated to this change.

[Aias00]

Ping on state: this PR is approved on the code, but pr_build fails and it cannot be merged yet. This one looks like a real regression, please take a look.

Failing tests in shenyu-admin:

org.apache.shenyu.admin.discovery.DefaultDiscoveryProcessorTest.testCreateSeparateDiscoveriesOfSameType
org.apache.shenyu.admin.discovery.DefaultDiscoveryProcessorTest.testRemoveProxySelector
org.apache.shenyu.admin.discovery.DefaultDiscoveryProcessorTest.testRemoveProxySelectorAfterDiscoveryRemoved
org.apache.shenyu.admin.discovery.DefaultDiscoveryProcessorTest.testChangeUpstream

All four are in the discovery processor, which sits directly on top of the classloader-isolated extension loading this PR changes. Please run shenyu-admin tests locally and either fix the cause or tell us the failures are order-dependent flakes, in which case we will rerun them here.

[Aias00]

@dengliming This PR is approved, but the pr_build check is red. I looked at that job on several PRs today and it fails in shared test setup rather than in the diff: the MQTT SubscribeTest fixture throws NullPointerException: Cannot invoke "Object.hashCode()" because "key" is null at SubscribeTest.java:75/90 because the repositories are not registered in Singleton.INST (#8075 fixes exactly that), and another variant dies on a stale shenyu-register-client-beat artifact.

Please rebase onto master and push so CI re-runs. If pr_build is still red afterwards, paste the failing test and I'll take it from there.

[Aias00]

@dengliming — a correction to the note I left here yesterday. The failures on this branch all predate the fix, so nothing in your change is implicated.

master went green at a8080ae: the SubscribeTest fixture isolation (#8092), the MQTT session scoping (#8093) and the pr_build dependency fix (#8075) all landed, and the latest ci.yml, integrated-test.yml and CodeQL runs on master are all success.

That means the red checks recorded here are stale. A push — an empty commit or a rebase, whichever is easier — re-runs them against the current baseline. I would not spend time reading the old logs.

[Aias00]

Hi @dengliming, this PR is already approved, but it currently cannot be merged. Here is the blocker and what is needed:

Blocker
This PR is approved but its own change breaks existing tests. pr_build fails in shenyu-admin with 11 test errors in the discovery tests:

• DefaultDiscoveryProcessorTest - 7 errors: NullPointerException: Cannot invoke "org.apache.shenyu.spi.ExtensionLoader.getExtensionClasses1()" because "loader" is null, and MockitoException: static mocking is already registered in the current thread at DefaultDiscoveryProcessorTest.java:107/113.
• NacosDiscoveryRecoveryTest.setUp:120 - 3 errors, same ExtensionLoader NPE.
• ZookeeperClientDiscoveryIntegrationTest.testClientRegistrationAndInstanceChanges - 1 error.

This is directly related to the SPI classloader isolation in this PR: the tests static-mock ExtensionLoader, and the new loader lookup returns null for them.

What we need
Please make the SPI isolation backward-compatible with the existing static-mock-based tests (or update those tests to the new accessor) and push a fix.

Once the branch is up to date and CI is green, we will squash-merge it. Thanks!

[Aias00]

Reviewed SPI loader separation by ClassLoader and registry/web loader callers. No actionable finding.

Validation: 11 focused SPI/registry tests passed; an additional web-loader reactor run passed 3 tests (including 2 repeated SPI tests). Apache RAT passed locally. Scope: source review and targeted verification; full integration/e2e suites were not run locally for this review.