On Wed, 18 Jun 2025 12:53:56 GMT, David Beaumont <d...@openjdk.org> wrote:
> Simplifying JavaRuntimeURLConnection to avoid accidentally returning
> non-resource data to users.
>
> This change has the following distinct parts:
> 1. Refactor code to use Node instead of directly accessing low level
> ImageLocation type.
> 2. Remove unnecessary use of "Resource" interface and related URL generation
> code (completely unreachable).
> 3. Adding comments explaining why there's a non-obvious distinction in how
> module and resource names are treated with respect to URL percent encoding.
> 4. Small constructor logic simplification (module name cannot be null anymore)
> 5. Small simplification around 'READER' use, since it is impossible for that
> to ever be null (other users of ImageReaderFactory already assume it could
> never be null, and code path analysis agrees).
> 6. Adding tests for the non-resource cases.
> 7. Adding extra test data to check the behaviour with respect to things like
> percent escaping (previously untested).
> 8. Adding TODO comments for things I could do in this PR or later (reviewer
> opinions welcome).
Preloading a few explanatory comments.
src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
line 49:
> 47:
> 48: // ImageReader to access resources in jimage
> 49: private static final ImageReader reader =
> ImageReaderFactory.getImageReader();
Examination of code and other users shows that this could never be null. Other
users also name it "READER".
src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
line 56:
> 54:
> 55: // the Resource when connected
> 56: private volatile Resource resource;
The Resource API was never needed here and adds uncalled methods such as
`getURL()` which are then implemented with code that could never be invoked.
src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
line 64:
> 62: throw new MalformedURLException(url + " missing path or /");
> 63: if (path.length() == 1) {
> 64: this.module = null;
There's no reason for the module==null case, since the only thing caring that
this might be null just converts null to the empty string anyway.
src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
line 101:
> 99: }
> 100: this.resource = node;
> 101: super.connected = true;
I know I don't *need* to use super here, but it documents the fact that this is
not a field of this subclass, without readers having to go check. Mutable
protected fields are weird and error prone, so I felt calling it out a bit was
worth it. Happy to replace with a comment if people feel that's better though.
src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
line 119:
> 117: public long getContentLengthLong() {
> 118: try {
> 119: return getResourceNode().size();
Having getResourceNode() return the (lazily fetched) node avoids the reader
needing to know/reason about how "connect()" has the side-effect of making the
"resource" field non-null.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25871#pullrequestreview-2939094081
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154527602
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154529805
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154532882
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154538127
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154543346
