MemoryResource #8451
MemoryResource #8451
Conversation
Added MemoryResource to simplify favicon handling
| @Override | ||
| public Resource resolve(String subUriPath) | ||
| { | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
resolve() should never throw, otherwise we're going to have to add try / catch blocks everywhere we call it. I'm not even sure we should let it return null, so it feels like the right thing to do here is to return this.
There was a problem hiding this comment.
I think this is the wrong thing to return.
We don't want these resources slipping into general usage (not unless we implement an in memory filesystem... and hey that is what ZipFS is), so I'm OK with an unchecked exception. If one of these resources ever ends up being used for a resolve, then that is a bad state and should fail.
There was a problem hiding this comment.
It has a URI, so I'm now just defering to the default resolve. You wont get back a memory resource, but that is fine.
| @Override | ||
| public Path getPath() | ||
| { | ||
| return null; |
There was a problem hiding this comment.
So far, code has been written that assumes Resource.getPath() cannot return null, and maybe there's some code that still calls File.newInputStream(resource.getPath()) instead of resource.getInputStream().
I think we should arrange for this implementation of getPath() to never return null, maybe by creating a simple in-memory filesystem?
There was a problem hiding this comment.
maybe by creating a simple in-memory filesystem?
heh, uh, this is a BIG undertaking.
There was a problem hiding this comment.
@lorban Well every time I see anybody saying that getPath cannot return null, I've said "oh yes it can". If getPath could always return non-null then we would have succeeded in replacing Resource with Path as there would be a 1:1 mapping. But there is not. ResourceCollection is one such resource that cannot always return a Path.
I've been on the lookout for code that makes that kind of assumption (eg getting the Path and using it to get last modified, rather than just asking the resource). We need to remove such assumptions. We should javadoc that getPath can return null.
There was a problem hiding this comment.
maybe by creating a simple in-memory filesystem?
heh, uh, this is a BIG undertaking.
Yeah - that's why I want this to be just a single resource - no lists, no directories, no resolve.
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/MemoryResource.java
Outdated
Show resolved
Hide resolved
| try | ||
| { | ||
| _uri = Objects.requireNonNull(url).toURI(); | ||
| _bytes = IO.readBytes(url.openStream()); |
There was a problem hiding this comment.
You're leaking a file descriptor.
There was a problem hiding this comment.
I believe IO closes it, by I'll use a try with resources.
jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/MemoryResourceTest.java
Show resolved
Hide resolved
|
Whatever you decide for The way it's handled in Jetty 12 now is that those exist as mounts on the Server object, which already has the |
I'm doing this for |
Updates from review
|
Wouldn't it be easier to copy those to the work dir and serve them via a normal file? |
Potentially, but remember our start times are important as well. We don't want to mount a zip file, extract favicon, unmount it, mount it again, extract style sheet, unmount etc. So that ends up with the result we want but will take a lot of CPU cycles at start time. These are classloader resources already available via the classloader getResource mechanism... unfortunately via a URL. So we just want to quickly grab the contents via the existing JVM mechanism and make them available. |
|
The unmount only occurs at server.stop() for those resources |
|
@joakime if we use the zipFS bases mechanism to get these resources, then either we leave it mounted whilst running (wasting memory) or we unmount after extracting (wasted CPU). There is already a perfectly good (modulo URL API) mechanism to get classpath resources. We should use that and not re-crack open the jar file. The zipFS mechanism behind Resource is great and we should use it... but only when appropriate (eg the deployer has asked to not unpack a war file). We should not use it when a better mechanism already exists. |
|
This is adequate for now, but we should come up with an agreement for the long term. That discussion should happen in #8443. |
Added MemoryResource to simplify favicon handling.
Using the ResourceFactory mechanism results in an entire jar mounted as a ZipFS in memory that is only needed for favicon.ico