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

Issue #11536 - ServletContext.getResourceAsStream(String) must use URLConnection #11537

Open
wants to merge 2 commits into
base: jetty-12.0.x
Choose a base branch
from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Mar 20, 2024

Making Implementations of ServletContext.getResourceAsStream(String) use URLConnection properly (per spec) in ee10/ee9 codebases.

Fixes #11536

@joakime joakime added Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc) labels Mar 20, 2024
@joakime joakime requested review from gregw and lorban March 20, 2024 18:22
@joakime joakime self-assigned this Mar 20, 2024
Comment on lines +2835 to +2838
// Per servlet rules, we have to use URLConnection to return this stream.
URLConnection urlConnection = url.openConnection();
urlConnection.setUseCaches(false);
return urlConnection.getInputStream();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janbartel is not at her computer, but has pointed out that the javadoc does not require URLConnection to be used? I really think we should avoid this as once URLs are used, then the URL cache mechanism is engaged and we loose control over jar caching.

What makes you think we should use URLConnection?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the javadoc on ServletContext.getResourceAsStream(String)

Which all say ...

    * <p>
    * The servlet container must implement the URL handlers and <code>URLConnection</code> objects necessary to access
    * the resource.

Also note that url caching is disabled in the impl on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime I think you are reading that the wrong way. It says that if a URL handler is necessary to access a resource, then the server must implement it. But nothing says that the server must use a URL to access resource that a URL handler is not necessary for.

We really do not want to do this and re-open the whole can of worms that is the caching done by the URL API. The big selling point of switching to PathResource was precisely that we could control the mounting and unmounting of the zipfs used to access jar resources. If we start using the URL API again, then we lose that control over memory again.

Specifically, for the issue associated with this... using URL is more likely to lock a file in the filesystem than not using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary for other issue, just something I noticed while troubleshooting it.
Moved this PR (and issue) to 12.0.9, as it needs further analysis.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joakime yeah, let's have a bit more of a think about this one. I'm not convinced that the spec is/should dictate that the InputStream must be obtained from a URLConnection - the words are bit open to interpretation, and the contract of the method signature is purely InputStream. Not sure I really want to reopen the whole URLConnection cache can-of-worms again - didn't taste great the last time around ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

ServletContext.getResourceAsStream(String) must use URLConnection per Servlet Spec.
3 participants