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

util: Add support for GraalVM Native-Image resource:-URIs and Paths #9136

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,6 @@
public final class URIUtil
{
private static final Logger LOG = LoggerFactory.getLogger(URIUtil.class);
private static final Index<String> KNOWN_SCHEMES = new Index.Builder<String>()
.caseSensitive(false)
.with("file:")
.with("jrt:")
.with("jar:")
.build();

// From https://www.rfc-editor.org/rfc/rfc3986
private static final String UNRESERVED = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-._~";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
//
// ========================================================================
// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.util.resource;

import java.net.URI;
import java.nio.file.Files;
import java.nio.file.Path;

/**
* GraalVM Native-Image {@link Path} Resource.
*/
public class NativeImagePathResource extends PathResource
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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.

{
private static final String URI_BAD_RESOURCE_PREFIX = "file:///resources!";

NativeImagePathResource(Path path, URI uri, boolean bypassAllowedSchemeCheck)
{
super(path, correctResourceURI(uri), (bypassAllowedSchemeCheck || isResourceScheme(uri)));
}

private static final boolean isResourceScheme(URI uri)
{
return "resource".equals(uri.getScheme());
}

/**
* Corrects any bad {@code resource} based URIs, such as those starting with {@code resource:file:///resources!}.
*
* @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>
Copy link
Contributor

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.

*/
static URI correctResourceURI(URI uri)
{
if (uri == null || !isResourceScheme(uri))
return uri;

String ssp = uri.getSchemeSpecificPart();
if (ssp.startsWith(URI_BAD_RESOURCE_PREFIX))
{
return URI.create("resource:" + ssp.substring(URI_BAD_RESOURCE_PREFIX.length()));
}
else
{
return uri;
}
}

@Override
public URI getRealURI()
{
Path realPath = getRealPath();
return (realPath == null) ? null : correctResourceURI(realPath.toUri());
}

@Override
protected URI toUri(Path path)
{
URI pathUri = correctResourceURI(path.toUri());
String rawUri = pathUri.toASCIIString();

if (Files.isDirectory(path) && !rawUri.endsWith("/"))
{
return URI.create(rawUri + '/');
}
return pathUri;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
//
// ========================================================================
// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.util.resource;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.FileSystemNotFoundException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;

/**
* GraalVM Native-Image {@link PathResourceFactory}.
*/
public class NativeImagePathResourceFactory extends PathResourceFactory
Copy link
Contributor

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

Copy link
Contributor

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?

{
static final boolean ENABLE_GRAALVM_RESOURCE_SCHEME = (System.getProperty("org.graalvm.nativeimage.kind") != null);
Copy link
Contributor

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)


public NativeImagePathResourceFactory()
{
if (ENABLE_GRAALVM_RESOURCE_SCHEME)
{
initNativeImageResourceFileSystem();
}
}

private void initNativeImageResourceFileSystem()
{
try
{
URI uri = new URI("resource:/");
try
{
Path.of(uri);
}
catch (FileSystemNotFoundException e)
{
FileSystems.newFileSystem(uri, Collections.emptyMap());
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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);

Copy link
Contributor

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

}
}

@Override
public Resource newResource(URI uri)
{
uri = NativeImagePathResource.correctResourceURI(uri.normalize());
Path path = Path.of(uri);

if (!Files.exists(path))
return null;

return new NativeImagePathResource(path, uri, false);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,10 @@ public void copyTo(Path destination) throws IOException
* @param path the path to convert to URI
* @return the appropriate URI for the path
*/
private static URI toUri(Path path)
protected URI toUri(Path path)
{
URI pathUri = path.toUri();
String rawUri = path.toUri().toASCIIString();
String rawUri = pathUri.toASCIIString();

if (Files.isDirectory(path) && !rawUri.endsWith("/"))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ class ResourceFactoryInternals
PathResourceFactory pathResourceFactory = new PathResourceFactory();
RESOURCE_FACTORIES.put("file", pathResourceFactory);
RESOURCE_FACTORIES.put("jrt", pathResourceFactory);

if (NativeImagePathResourceFactory.ENABLE_GRAALVM_RESOURCE_SCHEME)
RESOURCE_FACTORIES.put("resource", new NativeImagePathResourceFactory());
Copy link
Contributor

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.

Copy link
Contributor

@joakime joakime Jan 9, 2023

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

}

static ResourceFactory ROOT = new CompositeResourceFactory()
Expand Down