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

Reply via email to