-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
util: Add support for GraalVM Native-Image resource:-URIs and Paths #9136
util: Add support for GraalVM Native-Image resource:-URIs and Paths #9136
Conversation
GraalVM Native-Image makes its classpath resources accessible through an opaque resource: URL scheme. Add support for this scheme in URIUtil and PathResource. Automatically create the NativeImageResourceFileSystem when necessary. Also add a workaround where converting such Native-Image Paths back to URI would yield the wrong URI (potentially a bug in GraalVM). jetty#9116 oracle/graal#5720 Signed-off-by: Christian Kohlschütter <[email protected]>
8981692
to
b53e7e0
Compare
Github CodeQL code scanning reports a high-severity error "Uncontrolled data used in path expression", because a path depends on a user-provided value. This is a false positive. Suppress the error by annotating a corresponding @SuppressWarnings tag. Signed-off-by: Christian Kohlschütter <[email protected]>
50334d0
to
41c80b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having resource
scheme be enabled by default is unsafe for the majority of users (esp those outside of GraalVM).
Why can't this exist as it's own ResourceFactory?
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Outdated
Show resolved
Hide resolved
This PR breaks OSGI users. (which also register |
This default list of schemes is carefully selected. We support:
The rest are problematic on different runtimes and must be configured on a case by case basis, not defaulted. This is the problematic list:
These should be provided by a unique ResourceFactory that is registered via ResourceFactory.registerResourceFactory(String scheme, ResourceFactory factory); |
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,7 @@ | |||
PathResourceFactory pathResourceFactory = new PathResourceFactory(); | |||
RESOURCE_FACTORIES.put("file", pathResourceFactory); | |||
RESOURCE_FACTORIES.put("jrt", pathResourceFactory); | |||
RESOURCE_FACTORIES.put("resource", pathResourceFactory); // GraalVM native-image resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shall not be automatic or defaulted.
The modifications you have made to URIUtil
and PathResource
should be refactored out into a standalone GraalVMResourceFactory
implementation, not by modifying the default behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about that, but then GraalVM-unaware code that simply tries to serve resources from the classpath will fail.
I've amended my change to transparently support the following real-world use case:
URL resourceURL = MyClass.class.getResource("some/resource");
Resource resource = ResourceFactory.root().newResource(resourceURL);
The new change only enables "resource:" URLs when a well-known GraalvM Native Image system property is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can go one of two ways:
- automatic discovery of unknown schemes. We have always done this for URL handlers, but now it looks like we'd also need to do an attempt to to look for a filesystem for a Path based unknown scheme provided by the JVM. That makes me less inclined to go automatic.
- A simple registration mechanism, that can easily be put into a module. Potentially fairly generic so it can add either URL handlers or FileSystem mounters for an arbitrary resource. A graalvm module would then do the mapping for "resource" to the FileSystem mounting extension of PathResourceFactory.
KNOWN_SCHEMES isn't used anywhere.
In regular HotSpot VMs, the resource: scheme may be registered by other clients. However, in GraalVM Native Image, this is used for classpath resources. This resource scheme needs to be enabled by default for Native Image environments so we can support code that is unaware of GraalVM internals, such as: URL resourceURL = MyClass.class.getResource("some/resource"); Resource resource = ResourceFactory.root().newResource(resourceURL); Signed-off-by: Christian Kohlschütter <[email protected]>
Ignore FileSystemAlreadyExistsException, ProviderNotFoundException, etc. that may be thrown upon calling FileSystems.newFileSystem. Signed-off-by: Christian Kohlschütter <[email protected]>
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Fixed
Show fixed
Hide fixed
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Fixed
Show fixed
Hide fixed
0aa2fce
to
bcf583c
Compare
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
Fixed
Show fixed
Hide fixed
bcf583c
to
8232d99
Compare
CodeQL prevents amending the call to Paths.get. Work-around this by using a separate constructor. The additional benefit is that URIUtil.correctResourceURI won't need to correct the URI twice. Signed-off-by: Christian Kohlschütter <[email protected]>
8232d99
to
0bb9d16
Compare
Fix two more places where Native Image resource: URIs need to be changed. Without this change, Resource#isAlias() would return true for such URIs, and a warning like "BaseResource resource:/some/package/ is aliased to resource:file:///resources!/some/package/" would be logged.
@joakime I'm still not convinced that it is "dangerous" for us to support arbitrary URL schemes? We've done so up until jetty 11 without issue. I'm not totally opposed to having to register exotic schemes, but then I'm also not opposed to automatic discovery either. Let's schedule a meeting this week where you can outline exactly why you are advocating a new policy of no automatic scheme discovery @kohlschuetter it's interesting that this scheme can be implemented with a PathResource. Would i be correct in assuming that in previous releases, the scheme was sorted by a discovered URL handler? If so, if we only had generic UrlResource handling enabled, would that work for Graal? Is there a performance penalty of going via URL rather than Path? @joakime i think we also need to discuss schemes that are supported by Path that we have not considered. Let's chat mid week. |
Note also, that if we can't support Graal out of the box (preferred outcome), then we should be able to have a module that simply adds/configures support. |
static final boolean ENABLE_GRAALVM_RESOURCE_SCHEME = (System.getProperty( | ||
"org.graalvm.nativeimage.kind") != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really don't like configuration by System property. Yes I know that we do it elsewhere in the code, but we are trying to remove them.
Either we should always attempt discovery of unknown schemes OR we should provide a simple way to add ResourceFactory
s if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this System Property come from?
Is it something that is always set by a GraalVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a System Property, is there a class/resource that can be requested that will return a URL with scheme resource:
that will work on all GraalVM runtimes?
That way startup could look for it, and auto-register a unique GraalVMResourceFactory
, if the returned URL is scheme resource
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The system property is set for all GraalVM Native-Image environments (i.e., binary executable or shared library). Since it's a property, it may also be set for testing purposes in a non-native (HotSpot) environment. The code is written in a way that should not break anything in that case (notwithstanding the potential OSGI issue with the resource:/ scheme that Oracle probably has to figure out somehow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite invasive, as we put another project's system property into common Jetty code.
I would prefer a Graal-specific ResourceFactory, along with anything else that Graal requires, isolated into a Jetty module as @gregw suggests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this check upon request because @joakime mentioned that OSGI would use resource:
for other purposes. I don't know exactly how always registering resource:
would be a problem though. It's of equivalent standard as jrt
, which also isn't always available.
Moreover, checking for the availability of classes in Native-Image comes at the cost of adding support for reflection, which needs additional configuration. I'd be happy to remove that check. @gregw mentioned earlier that there could be an interest to support arbitrary schemes / add discovery of new ones.
// initialize NativeImageResourceFileSystem, if necessary | ||
URI uri; | ||
try | ||
{ | ||
uri = new URI("resource:/"); | ||
try | ||
{ | ||
Path.of(uri); | ||
} | ||
catch (FileSystemNotFoundException e) | ||
{ | ||
FileSystems.newFileSystem(uri, Collections.emptyMap()); | ||
} | ||
} | ||
catch (IOException | URISyntaxException | RuntimeException ignore) | ||
{ | ||
// ignore | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like such specific initializer code in a fairly generic class. This would probably be better put in the constructor of a ResourceFactory
that is installed if needed. This would also allow parameters to be set and passed in if needed.
Note that the style of initialization could probably be generalized so that any JVM supported filesystem that needs to be created can be. i.e. it does not need to have "resource" hard coded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be able to move that initializer to all to a wrapper for Path.of
/Paths.get
that always tries to initialize the filesystem for the given URI upon FileSystemNotFoundException
.
One minor issue is that I haven't found a way to convince Github's CodeQL checker to not throw crazy errors at me when using a "user-supplied" parameter to Path.of
.
@gregw As I got sucked down the "path" of adding support for resources in GraalVM native-image, I feel adding a custom GraalVM ResourceFactory only makes sense if it's enabled by default in that environment. After all, this eventually will just be "another JVM", not something that the Jetty user has much control over. While accessing these resources almost always starts with a URL ( I can try moving most code into a To add some context, I'm working on a project that builds around an embedded Jetty server. With this patch set, I can run a full http/servlet environment as a single executable binary in less than 100 MB, including content and all. This makes Jetty an ideal candidate to run in a constrained virtual environment like Firecracker using Linux or even OSv. |
Keep the GraalVM Native-Image code contained in custom subclasses. We still enable the "resource:" URL scheme by default if a GraalVM Native-Image environment is detected via System property. This allows us to transparently support calls like ResourceFactory.root().newResource(MyClass.class.getResorce("webapp")) Signed-off-by: Christian Kohlschütter <[email protected]>
Looking back at Issue #9116, this started as support for It looks like GraalVM now supports I'd like to experiment a bit with this new reality. Lets use this project -> https://github.com/joakime/graalvm-experiments (I've added you as a collaborator if you want to muck with it there) How can I run this demo project with GraalVM that has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better.
I think we are down to just naming nits now.
@gregw WDYT?
* @param uri | ||
* The URI to correct. | ||
* @return the corrected URI, or the original URI. | ||
* @see <a href="https://github.com/oracle/graal/issues/5720">Graal issue 5720</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for linking to the bug here. It will help in the future.
*/ | ||
public class NativeImagePathResourceFactory extends PathResourceFactory | ||
{ | ||
static final boolean ENABLE_GRAALVM_RESOURCE_SCHEME = (System.getProperty("org.graalvm.nativeimage.kind") != null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could test a Classloader.getResource(reference)
for scheme resource
here too (instead of relying on a System property)
/** | ||
* GraalVM Native-Image {@link Path} Resource. | ||
*/ | ||
public class NativeImagePathResource extends PathResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this include the name GraalVM
in the beginning?
aka GraalVMNativeImagePathResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to avoid GraalVM specific classes.
However, am I correct is thinking that this class is only needed to work around the Graal bug. Once that bug is fixed, the factory can just create normal PathResource
instances?
If so, then maybe even call this GraalIssue5720PathResource
to make it very clear why it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this class doesn't need to exist once the GraalVM native-image bugs are fixed.
/** | ||
* GraalVM Native-Image {@link PathResourceFactory}. | ||
*/ | ||
public class NativeImagePathResourceFactory extends PathResourceFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this include the name GraalVM
in the beginning?
aka GraalVMNativeImagePathResourceFactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd very much like to avoid GraalVM specific classes.
This class is could be made generic for any scheme that is supported by Path and a FileSystem if the scheme was parameterized.
So perhaps parameterize the scheme and name it something like FileSystemResourceFactory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good and we'll get it merged one way or another.
I'm on the fence about which way to go to compete this. Either folow @joakime's suggestions of renaming to GraalVMNativeImageXxx
classes; OR make it generic so that any VM supplied FileSystem type can be supported against an arbitrary scheme.
Perhaps the solution is to make a generic base factory that is parameterized over scheme and FileSystem configuration properties. Then have a GraalVM specific extension that is instantiated for "resource" and has the bug work around?
Or is that trying to support generic N from an example of 1? @joakime thoughts?
/** | ||
* GraalVM Native-Image {@link Path} Resource. | ||
*/ | ||
public class NativeImagePathResource extends PathResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to avoid GraalVM specific classes.
However, am I correct is thinking that this class is only needed to work around the Graal bug. Once that bug is fixed, the factory can just create normal PathResource
instances?
If so, then maybe even call this GraalIssue5720PathResource
to make it very clear why it exists.
/** | ||
* GraalVM Native-Image {@link PathResourceFactory}. | ||
*/ | ||
public class NativeImagePathResourceFactory extends PathResourceFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd very much like to avoid GraalVM specific classes.
This class is could be made generic for any scheme that is supported by Path and a FileSystem if the scheme was parameterized.
So perhaps parameterize the scheme and name it something like FileSystemResourceFactory
?
} | ||
catch (FileSystemNotFoundException e) | ||
{ | ||
FileSystems.newFileSystem(uri, Collections.emptyMap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was a generic class, then we should allow parameters to be configured.
} | ||
catch (IOException | URISyntaxException | RuntimeException ignore) | ||
{ | ||
// ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we log exceptions as warnings here, as they may be thrown by newFileSystem
and that feels significant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, since there's nothing we can do here.
If any of this fails, subsequent attempts to Path.of
/Paths.get
a resource:
URI will throw an exception from within the calling stack.
We try-catch RuntimeException
for example to ignore a race condition between us trying to initialize the filesystem and application code that triggers the filesystem initialization independently (we might get a FileSystemAlreadyExistsException
just at an inopportune time);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at least a debug log would be helpful. If somebody is getting URI exceptions elsewhere because the filesystem didn't mount, it would be good to be able to see the exceptions without needing to modify code
if (NativeImagePathResourceFactory.ENABLE_GRAALVM_RESOURCE_SCHEME) | ||
RESOURCE_FACTORIES.put("resource", new NativeImagePathResourceFactory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still cautious about this. If this was not a VM feature, then I would definitely not like this.
The alternative is to have a module that adds in this mapping "resource" to the resource factory, but then embedded code would still need to do a registration explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could easily add other Path/FileSystem implementation schemes automatically here.
I would probably access something in Jetty that is known, and use it's protocol/scheme to register other schemes at this point in time.
Eg:
ClassLoader classLoader = Thread.currentThread().getContextClassLoader();
URL url = classLoader.getResource("org/eclipse/jetty/version/build.properties");
if (url != null)
{
RESOURCE_FACTORIES.put(url.getProtocol(), pathResourceFactory);
}
In the future where the GraalVM bug is fixed, this would be enough to make native-image work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't a newFileSystem
call still needed somewhere? Doesn't need to be in a ResourceFactory
but is still needed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newFileSystem
call is now made from within the NativeImagePathResourceFactory
constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw AFAICT, there is only 1 root FileSystem for the resource:
scheme, representing all of the content in the entire native-image.
So having the NativeImagePathResourceFactory
control the mount makes sense here.
But a more general approach (for any other FileSystem types) is definitely something to seek out.
@gregw since this is basically a work around for a bug in the FileSystem implementation in GraalVM native-image. What about having this as a standalone jar, at https://github.com/jetty-project/graalvm-native-image-bug-resource-factory ? In the future, when the GraalVM native-image bug is fixed, there only needs to be a small change to See: https://github.com/eclipse/jetty.project/pull/9136/files#r1065133573 |
I would advise against creating a new jar:
I'm OK with renaming the classes to Regarding @joakime's suggestion to replace the System property check with a call to |
Detect the resource: scheme by checking the scheme of a well-known resource instead of looking at a system property. Rename NativeImagePathResource* to GraalIssue5720PathResource* and make these classes package-private. Signed-off-by: Christian Kohlschütter <[email protected]>
e28b9f0
to
8d0d327
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to make this work even in the future, once the graalvm native-image bug is fixed.
See comments for suggestions on how to improve this.
static | ||
{ | ||
URL url = GraalIssue5720PathResourceFactory.class.getResource("/org/eclipse/jetty/version/build.properties"); | ||
ENABLE_NATIVE_IMAGE_RESOURCE_SCHEME = (url != null && "resource".equals(url.getProtocol())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this to be true, the bug (eg: file
or resource!
is present in Path.toURI()
) should be detected as well.
Can you do that here? (followup in ResourceFactoryInternals related to this request as well).
We don't want this ResourceFactory to be used once the bug is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would still want to enable "resource" as a scheme in ResourceFactoryInternals
, which is what this boolean controls...
Keep in mind that I started out with deliberately not writing a subclass, and instead use a method that simply fixes the URIs if they have a resource!
substring, which is a no-op once graal fixes this upstream.
I'm happy to entertain this PR a little longer but I wonder if we're chasing a moving goal.
@@ -0,0 +1,9 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need several dozen more of these scattered around the codebase for this concept.
Can this have a comment indicating it's for GraalVM native-image support?
Frankly, I'm leaning more and more towards a jetty-graalvm-native-image
support jar, which has all of these things in it, including the bug workaround resource factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these json files sadly don't accept comments.
I'm also afraid that yes, for native-image you will need a couple more native-image json files in your project. This isn't much of a problem, though, once you get accommodated with the fact that there's this new concept of ahead-of-time-compiled native binaries from Java code.
In my humble opinion, the whole support-jar concept is flawed due to the way (Maven) dependencies are determined.
As a user of some feature that pulls in a Java dependency (in this case, Jetty), I don't really want to deal with separate artifacts for different JVM implementations.
You see a comparable case with multi-release jars, which are so much better than having per-version jars. It just makes more sense to embrace the capabilities of META-INF
-based configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we need to expose META-INF/services
files with this resource-config.json
too?
Do you know of a maven plugin that can auto-generate these files during the build?
Checking the rest of Jetty 12, I see ...
[jetty-12.0.x]$ find . -type f -wholename "*/src/main/resources/*" | grep -v demo | grep -v jetty-home | grep -v "\-test" | grep -v "test-"
./jetty-ee8/jetty-ee8-glassfish-jstl/src/main/resources/readme.txt
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-effective-web-xml-it/resources/src/main/resources/META-INF/resources/extra.html
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-war-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-start-war-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/it/jetty-run-mojo-jar-scan-it/MyLibrary/src/main/resources/META-INF/services/javax.servlet.ServletContainerInitializer
./jetty-ee8/jetty-ee8-maven-plugin/src/main/resources/ee8-maven.mod
./jetty-ee8/jetty-ee8-maven-plugin/src/main/resources/maven-ee8.xml
./jetty-ee8/jetty-ee8-maven-plugin/src/main/resources/jetty-ee8-maven.xml
./jetty-ee8/jetty-ee8-webapp/src/main/resources/org/eclipse/jetty/ee8/webapp/catalog-ee8.xml
./tests/jetty-jmh/src/main/resources/jetty-logging.properties
./documentation/jetty-asciidoctor-extensions/src/main/resources/META-INF/services/org.asciidoctor.jruby.extension.spi.ExtensionRegistry
./build/build-resources/src/main/resources/jetty-checkstyle.xml
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-server/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-server/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jakarta-server/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jakarta-server/src/main/resources/META-INF/services/jakarta.websocket.server.ServerEndpointConfig$Configurator
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jakarta-server/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-client/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jetty-common/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.websocket.api.ExtensionConfig$Parser
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jakarta-client/src/main/resources/META-INF/services/jakarta.websocket.ContainerProvider
./jetty-ee9/jetty-ee9-websocket/jetty-ee9-websocket-jakarta-client/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-plus/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-apache-jsp/src/main/resources/META-INF/services/org.apache.juli.logging.Log
./jetty-ee9/jetty-ee9-apache-jsp/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-osgi/jetty-ee9-osgi-boot/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-annotations/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-effective-web-xml-it/resources/src/main/resources/META-INF/resources/extra.html
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-war-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-start-war-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/it/jetty-run-mojo-jar-scan-it/MyLibrary/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-maven-plugin/src/main/resources/jetty-ee9-maven.xml
./jetty-ee9/jetty-ee9-maven-plugin/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-maven-plugin/src/main/resources/ee9-maven.mod
./jetty-ee9/jetty-ee9-maven-plugin/src/main/resources/maven-ee9.xml
./jetty-ee9/jetty-ee9-quickstart/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-jaspi/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.security.Authenticator$Factory
./jetty-ee9/jetty-ee9-openid/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.security.Authenticator$Factory
./jetty-ee9/jetty-ee9-jspc-maven-plugin/src/main/resources/META-INF/m2e/lifecycle-mapping-metadata.xml
./jetty-ee9/jetty-ee9-cdi/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-cdi/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee9/jetty-ee9-runner/src/main/resources/META-INF/MANIFEST.MF
./jetty-ee9/jetty-ee9-runner/src/main/resources/jetty-logging.properties
./jetty-ee9/jetty-ee9-nested/src/main/resources/org/eclipse/nested/favicon.ico
./jetty-ee9/jetty-ee9-nested/src/main/resources/jetty-dir.css
./jetty-ee9/jetty-ee9-ant/src/main/resources/tasks.properties
./jetty-ee9/jetty-ee9-ant/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-webapp/src/main/resources/META-INF/services/org.eclipse.jetty.ee9.webapp.Configuration
./jetty-ee9/jetty-ee9-webapp/src/main/resources/org/eclipse/jetty/ee9/webapp/catalog-ee9.xml
./jetty-ee9/jetty-ee9-glassfish-jstl/src/main/resources/readme.txt
./jetty-integrations/jetty-infinispan/jetty-infinispan-common/src/main/resources/session.proto
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-effective-web-xml-it/resources/src/main/resources/META-INF/resources/extra.html
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-war-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-distro-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-start-war-forked-mojo-it/jetty-simple-base/src/main/resources/META-INF/web-fragment.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/it/jetty-run-mojo-jar-scan-it/MyLibrary/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-maven-plugin/src/main/resources/maven-ee10.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-maven-plugin/src/main/resources/jetty-ee10-maven.xml
./jetty-ee10/jetty-ee10-maven-plugin/src/main/resources/ee10-maven.mod
./jetty-ee10/jetty-ee10-jspc-maven-plugin/src/main/resources/META-INF/m2e/lifecycle-mapping-metadata.xml
./jetty-ee10/jetty-ee10-openid/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.servlet.security.Authenticator$Factory
./jetty-ee10/jetty-ee10-glassfish-jstl/src/main/resources/readme.txt
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jakarta-client/src/main/resources/META-INF/services/jakarta.websocket.ContainerProvider
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jakarta-client/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jetty-server/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jetty-server/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jetty-common/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.websocket.api.ExtensionConfig$Parser
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jakarta-server/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jakarta-server/src/main/resources/META-INF/services/jakarta.websocket.server.ServerEndpointConfig$Configurator
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jakarta-server/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-websocket/jetty-ee10-websocket-jetty-client/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-cdi/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-cdi/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-plus/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-runner/src/main/resources/META-INF/MANIFEST.MF
./jetty-ee10/jetty-ee10-runner/src/main/resources/jetty-logging.properties
./jetty-ee10/jetty-ee10-annotations/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-jaspi/src/main/resources/META-INF/services/org.eclipse.jetty.security.Authenticator$Factory
./jetty-ee10/jetty-ee10-osgi/jetty-ee10-osgi-boot/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-ant/src/main/resources/tasks.properties
./jetty-ee10/jetty-ee10-ant/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-quickstart/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-apache-jsp/src/main/resources/META-INF/services/org.apache.juli.logging.Log
./jetty-ee10/jetty-ee10-apache-jsp/src/main/resources/META-INF/services/jakarta.servlet.ServletContainerInitializer
./jetty-ee10/jetty-ee10-webapp/src/main/resources/META-INF/services/org.eclipse.jetty.ee10.webapp.Configuration
./jetty-ee10/jetty-ee10-webapp/src/main/resources/org/eclipse/jetty/ee10/webapp/catalog-ee10.xml
./jetty-core/jetty-http/src/main/resources/META-INF/services/org.eclipse.jetty.http.HttpFieldPreEncoder
./jetty-core/jetty-http/src/main/resources/org/eclipse/jetty/http/encoding.properties
./jetty-core/jetty-http/src/main/resources/org/eclipse/jetty/http/mime.properties
./jetty-core/jetty-http/src/main/resources/org/eclipse/jetty/http/useragents
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/catalog-configure.xml
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/datatypes.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/xml.xsd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_7_6.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_9_0.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/catalog-org.w3.xml
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_6_0.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_9_3.dtd
./jetty-core/jetty-xml/src/main/resources/org/eclipse/jetty/xml/XMLSchema.dtd
./jetty-core/jetty-http-spi/src/main/resources/META-INF/services/com.sun.net.httpserver.spi.HttpServerProvider
./jetty-core/jetty-alpn/jetty-alpn-conscrypt-client/src/main/resources/META-INF/services/org.eclipse.jetty.io.ssl.ALPNProcessor$Client
./jetty-core/jetty-alpn/jetty-alpn-java-client/src/main/resources/META-INF/services/org.eclipse.jetty.io.ssl.ALPNProcessor$Client
./jetty-core/jetty-alpn/jetty-alpn-java-server/src/main/resources/META-INF/services/org.eclipse.jetty.io.ssl.ALPNProcessor$Server
./jetty-core/jetty-alpn/jetty-alpn-conscrypt-server/src/main/resources/META-INF/services/org.eclipse.jetty.io.ssl.ALPNProcessor$Server
./jetty-core/jetty-start/src/main/resources/org/eclipse/jetty/start/build.properties
./jetty-core/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt
./jetty-core/jetty-deploy/src/main/resources/org/eclipse/jetty/deploy/lifecycle-bindings.txt
./jetty-core/jetty-http3/jetty-http3-qpack/src/main/resources/META-INF/services/org.eclipse.jetty.http.HttpFieldPreEncoder
./jetty-core/jetty-http2/jetty-http2-hpack/src/main/resources/META-INF/services/org.eclipse.jetty.http.HttpFieldPreEncoder
./jetty-core/jetty-util/src/main/resources/org/eclipse/jetty/version/build.properties
./jetty-core/jetty-slf4j-impl/src/main/resources/META-INF/services/org.slf4j.spi.SLF4JServiceProvider
./jetty-core/jetty-server/src/main/resources/org/eclipse/jetty/server/favicon.ico
./jetty-core/jetty-server/src/main/resources/org/eclipse/jetty/server/jetty-dir.css
./jetty-core/jetty-jndi/src/main/resources/jndi.properties
./jetty-core/jetty-quic/jetty-quic-quiche/jetty-quic-quiche-jna/src/main/resources/META-INF/services/org.eclipse.jetty.quic.quiche.QuicheBinding
./jetty-core/jetty-quic/jetty-quic-quiche/jetty-quic-quiche-foreign-incubator/src/main/resources/META-INF/services/org.eclipse.jetty.quic.quiche.QuicheBinding
./jetty-core/jetty-websocket/jetty-websocket-core-common/src/main/resources/META-INF/services/org.eclipse.jetty.websocket.core.Extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first modules I would worry about are ...
jetty-http
jetty-xml
jetty-server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, META-INF/services
would also need to be included in resource-config.json.
An initial list for native-image configurations can be obtained by running the Native Image Tracing Agent using non-native GraalVM on either your final product (perform some regular usage patterns, the files will be saved upon JVM termination) or by running all unit tests.
The agent will also determine reflection accesses (not all of them, unfortunately).
For my junixsocket project, I've created a small script along with instructions to determine these files programmatically via a selftest application. The selftest app essentially runs all unit tests on the same platform as your native-image target and so you can increase the chance of catching all necessary configurations (macOS/Linux/Windows).
Getting this started is a bit tedious but once the files are in place, users of your library won't have to worry anymore.
Regular expressions can be used to implement "wildcard" rules, but don't make them too wide so we only include what is provided by your own artifact, not the entire Java universe of unrelated resources.
(Note that all of this is out of scope for this PR)
|
||
if (GraalIssue5720PathResourceFactory.ENABLE_NATIVE_IMAGE_RESOURCE_SCHEME) | ||
RESOURCE_FACTORIES.put("resource", new GraalIssue5720PathResourceFactory()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole block can be replaced with a more general approach, that also supports the bug workaround, along with supporting other FileSystem schemes, and even the future when the GraalVM native-image bug is fixed ...
// The default resource factories
MountedPathResourceFactory mountedPathResourceFactory = new MountedPathResourceFactory();
RESOURCE_FACTORIES.put("jar", mountedPathResourceFactory);
PathResourceFactory pathResourceFactory = new PathResourceFactory();
RESOURCE_FACTORIES.put("file", pathResourceFactory);
RESOURCE_FACTORIES.put("jrt", pathResourceFactory);
URL url = ResourceFactoryInternals.class.getResource("/org/eclipse/jetty/version/build.properties");
if (url != null)
{
if (GraalIssue5720PathResourceFactory.hasBug(url))
RESOURCE_FACTORIES.put(url.getProtocol(), new GraalIssue5720PathResourceFactory());
else if (!RESOURCE_FACTORIES.contains(url.getProtocol()))
RESOURCE_FACTORIES.put(url.getProtocol(), mountedPathResourceFactory);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GraalIssue5720PathResourceFactory.hasBug(URL)
would need to check for protocol resource
, and then perform a path filesystem mount and test of Path.toURI()
to detect the bug.
Thankfully, this static list is only initialized once at the start of the JVM, so it's not a performance hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't see a reason for MountedPathResourceFactory
to exist. Maybe I'm missing a point but you could do this all with a single PathResourceFactory
. All you'd need to do is to wrap Path.of
(or Paths.get
) with a try-catch
block to find FileSystemNotFoundException
very much like I do in this demo code. You don't have to manually wrangle with exclamation points for jar URLs. Just create a newFileSystem
for the requested URI. It will work just fine.
Such PathResourceFactory
would be capable of supporting all Path
-providing URL schemes.
It would still be the place where would need to handle the "graal issue 5720"-specific manipulation, which is how it was in my original solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If GraalIssue5720PathResourceFactory
and GraalIssue5720PathResource
classes only exist because of the graal bug && line 56 will be deleted once it's fixed && and line 57 can be moved elsewhere from ResourceFactoryInternals
once it's fixed, then I'm ok with it.
What I would like us to do is to use this example to learn how to handle, in a generic way, any url scheme. So far we've now had 2 examples: bundleresource:
and resource:
. Have we used those to extract a meaningful generic mechanism? If so, can we document that please.
...ty-util/src/main/java/org/eclipse/jetty/util/resource/GraalIssue5720PathResourceFactory.java
Show resolved
Hide resolved
This is true of many other 3rd party integrations with Jetty, Graal is not unique or special in this case.
It's not always required, it's only required if your use case consists of all three: Jetty Server, with GraalVM, with native-image. Back in the Jetty 9 days we even had a special artifact for users of Oracle JVM running in Compact 3 profile - https://github.com/eclipse/jetty.project/tree/jetty-9.4.x/aggregates/jetty-all-compact3 |
I went ahead and added a PR to add tests for supporting alternate FileSystem implementations at PR #9149 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are probably refinements that can be made, but as we still have some thinking to do on the whole resource factory stuff, I think this PR is better merged and considered as part of the whole as we clean up resources for a beta release of 12.
As per @gregw
@joakime Any blocking changes or can this be merged? |
@kohlschuetter this PR has conflicts currently, and cannot be merged. You'll need to fix the conflicts. |
…tiveImageResourcePath2
Previously, we were always enabling this resource factory for GraalVM native-image environments. We now check if that's actually necessary. We fall back to MountedPathResourceFactory or PathResourceFactory, depending on whether the URL contains "!/" or not.
GraalVM Native-Image makes its classpath resources accessible through an opaque resource: URL scheme.
Add support for this scheme in URIUtil and PathResource.
Automatically create the NativeImageResourceFileSystem when necessary. Also add a workaround where converting such Native-Image Paths back to URI would yield the wrong URI (potentially a bug in GraalVM).
#9116 oracle/graal#5720
Signed-off-by: Christian Kohlschütter [email protected]