Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid illegal reflective access in CLI tests #26930

Closed

Conversation

dreis2211
Copy link
Contributor

Hi,

this is an attempt to get rid of illegal reflective access warnings in CLI tests. The idea is to use TomcatURLStreamHandlerFactory.disable(); and thus not registering any factory in URL that we would otherwise need to reset via reflection.

As this is part of the overall JDK 17 story under #26767, it should probably go into 2.5.x.

Let me know what you think.
Cheers,
Christoph

P.S.: I have this flaky / failing test that seems unrelated, but for completeness reasons:

:spring-boot-project:spring-boot-cli:test
    org.springframework.boot.cli.ReproIntegrationTests > securityDependencies()

General error during conversion: java.lang.IllegalArgumentException: Malformed \uxxxx encoding.

org.springframework.boot.cli.compiler.grape.DependencyResolutionFailedException: java.lang.IllegalArgumentException: Malformed \uxxxx encoding.
	at org.springframework.boot.cli.compiler.grape.AetherGrapeEngine.resolve(AetherGrapeEngine.java:298)
	at org.springframework.boot.cli.compiler.grape.AetherGrapeEngine.grab(AetherGrapeEngine.java:116)
        ...

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 16, 2021
@philwebb
Copy link
Member

I've got a feeling that reflection hack might have been because other tests had ended up setting it, perhaps indirectly via StandardRoot.registerURLStreamHandlerFactory(). @dreis2211 Did you try without the disable() call? I wonder why this specific test needed that. The code is 7+ years old so lots of things may have changed.

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Jun 16, 2021
@dreis2211
Copy link
Contributor Author

@philwebb I tried without it and it horribly fails.
image

Caused by: java.lang.Error: factory already defined
	at java.base/java.net.URL.setURLStreamHandlerFactory(URL.java:1228) ~[na:na]
	at org.apache.catalina.webresources.TomcatURLStreamHandlerFactory.<init>(TomcatURLStreamHandlerFactory.java:130) ~[tomcat-embed-core-9.0.46.jar:9.0.46]
	at org.apache.catalina.webresources.TomcatURLStreamHandlerFactory.getInstanceInternal(TomcatURLStreamHandlerFactory.java:53) ~[tomcat-embed-core-9.0.46.jar:9.0.46]
	at org.apache.catalina.webresources.TomcatURLStreamHandlerFactory.register(TomcatURLStreamHandlerFactory.java:77) ~[tomcat-embed-core-9.0.46.jar:9.0.46]
	at org.apache.catalina.webresources.StandardRoot.registerURLStreamHandlerFactory(StandardRoot.java:700) ~[tomcat-embed-core-9.0.46.jar:9.0.46]
	at org.apache.catalina.webresources.StandardRoot.initInternal(StandardRoot.java:682) ~[tomcat-embed-core-9.0.46.jar:9.0.46]
	at org.apache.catalina.util.LifecycleBase.init(LifecycleBase.java:136) ~[tomcat-embed-core-9.0.46.jar:9.0.46]

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 16, 2021
@philwebb
Copy link
Member

Thanks for trying that. It's strange that TomcatURLStreamHandlerFactory.instance is null but URL.factory is not. Probably a classloader issue since URL is in the base classloader.

I guess disable() ends up setting the instance, not calling URL and then later on StandardRoot.registerURLStreamHandlerFactory is no-op. So all things considered, I think calling disable() will work fine. LGTM.

@philwebb philwebb added type: task A general task and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Jun 16, 2021
@philwebb philwebb added this to the 2.5.x milestone Jun 16, 2021
@dreis2211
Copy link
Contributor Author

dreis2211 commented Jun 16, 2021

@philwebb I'm confused. Where did you see URL.factory being set?

I guess disable() ends up setting the instance, not calling URL and then later on StandardRoot.registerURLStreamHandlerFactory is no-op

Yes, that's the idea behind the PR.

@philwebb
Copy link
Member

Where did you see URL.factory being set

I didn't look in detail, but I guess that StandardRoot.registerURLStreamHandlerFactory was called in an earlier test. That triggers TomcatURLStreamHandlerFactory.register() which will call URL.setURLStreamHandlerFactory.

In a regular app the TomcatURLStreamHandlerFactory.instance null check is enough to stop that happening twice. I suspect here we've got some kind of classloader malarkey going on where TomcatURLStreamHandlerFactory.register() gets called, the URL has a factory set then the classloader is destroyed. Next time we get another classloader so TomcatURLStreamHandlerFactory.instance is null again but URL.factory is still set because it's part of the JVM base classloader.

@dreis2211
Copy link
Contributor Author

dreis2211 commented Jun 16, 2021

I didn't look in detail, but I guess that StandardRoot.registerURLStreamHandlerFactory was called in an earlier test.

That would surprise me in the CLI project. It might have been in other submodules, but then again they run in separate worker VMs (at least they should be) and thus don't interfere with each other.

@philwebb
Copy link
Member

Thanks @dreis2211. I've merged this to 2.5.x and forward to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants