8353662: Add test for non-local file URL fallback to FTP #24418

eirbjo · 2025-04-03T15:44:44Z

Please review this test-only PR which introduces testing of the unspecified but long-standing fallback to FTP for non-local files in the 'file' URL scheme.

This in preparation for the upcoming proposal to disable the feature by default in JDK-8353440.

Since we cannot reliably bind an FTP server to port 21, the test instead uses an HTTP proxy, binding to an ephemeral port. This menas we don't test non-proxy code paths. We still test that the FTP fallback is used, which is the key point here. (We aim to test file URL connections, not FTP URL connection internals)

An alternative here could be to just verify that the returned URLConnection is an instance of FtpURLConnection. However, I opted for an end-to-end test here, since the amount of extra code seems reasonable.

By temporarly moving this test to tier1, I was able to confirm this test runs green also on Windows GHA.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8353662: Add test for non-local file URL fallback to FTP (Enhancement - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24418/head:pull/24418
$ git checkout pull/24418

Update a local copy of the PR:
$ git checkout pull/24418
$ git pull https://git.openjdk.org/jdk.git pull/24418/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24418

View PR using the GUI difftool:
$ git pr show -t 24418

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24418.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-03T15:45:41Z

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-03T15:45:45Z

@eirbjo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8353662: Add test for non-local file URL fallback to FTP

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 271 new commits pushed to the master branch:

b5d2e25: 8354189: Remove JLI_ReportErrorMessageSys on Windows
efb5a80: 8353730: TestSubNodeFloatDoubleNegation.java fails with native Float16 support
8bb0ca4: 8353917: jnativescan: Simplify ClassResolver
... and 268 more: https://git.openjdk.org/jdk/compare/c50a0a1fc126a67528448b282bcfc375abfac142...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-04-03T15:46:13Z

@eirbjo The following labels will be automatically applied to this pull request:

core-libs
net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-04T04:51:11Z

Webrevs

AlanBateman · 2025-04-04T05:57:42Z

/label remove core-libs

openjdk · 2025-04-04T05:58:07Z

@AlanBateman
The core-libs label was successfully removed.

dfuch · 2025-04-04T10:19:29Z

test/jdk/sun/net/www/protocol/file/NonLocalFtpFallback.java

+    @Test
+    public void verifyNonLocalFtpFallback() throws Exception {
+        URL localURL = file.toUri().toURL();
+        URL nonLocalURL = new URL("file", "127.0.0.1", localURL.getFile());


The URL should not assume that the loopback is an IPv4 address. Or is this handled by the proxy?

The URL should not assume that the loopback is an IPv4 address. Or is this handled by the proxy?

Right. The following returns "127.0.0.1" for me, would that be the correct API to use?

InetAddress.getLoopbackAddress().getHostAddress()

Ok - I see that 127.0.0.1 should not be directly used. Would it work if we replaced that with a fake host name then?

Ok - I see that 127.0.0.1 should not be directly used. Would it work if we replaced that with a fake host name then?

Yeah, you're right, the FTP request never materializes (our proxy just swallows it). So we only use it to assert against what was requested from the proxy, we can use a fake host name.

Any preference/precedence for fake host names?

Generally looks good. "127.0.0.1" rings alarm bells because we're trying to make our tests independent of the IP versions supported by the machine they run on.

Generally looks good.

Does this mean you think the PR is good for approval now that the IP version aspect has been taken care of?

Or would you like more time/people to review this test-only enhancement?

Sorry - thanks for pinging me.
I tested the change in our CI and the new test seem stable. Approved.

eirbjo · 2025-04-04T10:40:24Z

@dfuch Tagnential observation: proxyServer.stop(2) will always wait 2 seconds here. I was assuming it would only do that if there was an ongoing exchange.

Looking at the ServerImpl class, it looks like finished is never set to true. Specifically, ServerImpl.Dispatcher::handleEvent does not set it to true since that happens before terminating is set to true.

How is HttpServer::stop expected to work? Is something wrong with my expectations that this should return quickly with no inflight exchanges?

dfuch · 2025-04-04T10:58:51Z

@dfuch Tagnential observation: proxyServer.stop(2) will always wait 2 seconds here. I was assuming it would only do that if there was an ongoing exchange.

Looking at the ServerImpl class, it looks like finished is never set to true. Specifically, ServerImpl.Dispatcher::handleEvent does not set it to true since that happens before terminating is set to true.

How is HttpServer::stop expected to work? Is something wrong with my expectations that this should return quickly with no inflight exchanges?

Interesting. That looks like a bug to me. stop() should probably look at exchangeCount too - but that will require some thinking to get the asynchronous stops to work correctly. Should be worth logging a bug :-)

eirbjo · 2025-04-04T11:06:15Z

Interesting. That looks like a bug to me. stop() should probably look at exchangeCount too - but that will require some thinking to get the asynchronous stops to work correctly. Should be worth logging a bug :-)

Let's see if this captures the essence:

https://bugs.openjdk.org/browse/JDK-8353720

EDIT: There was already a bug filed for this: https://bugs.openjdk.org/browse/JDK-8304065

eirbjo · 2025-04-11T12:48:34Z

Thanks you Daniel for running this through CI and for the review.

/integrate

openjdk · 2025-04-11T12:49:43Z

Going to push as commit cae7a20.
Since your change was applied there have been 271 commits pushed to the master branch:

b5d2e25: 8354189: Remove JLI_ReportErrorMessageSys on Windows
efb5a80: 8353730: TestSubNodeFloatDoubleNegation.java fails with native Float16 support
8bb0ca4: 8353917: jnativescan: Simplify ClassResolver
... and 268 more: https://git.openjdk.org/jdk/compare/c50a0a1fc126a67528448b282bcfc375abfac142...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-11T12:49:51Z

@eirbjo Pushed as commit cae7a20.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added core-libs net labels Apr 3, 2025

eirbjo changed the title ~~Add a test verifying non-local file: URL fallback to FTP~~ 8353662: Add test for non-local file URL fallback to FTP Apr 3, 2025

Add a test verifying non-local file: URL fallback to FTP

f235d79

eirbjo force-pushed the non-local-file-ftp-test branch from 461f1c3 to f235d79 Compare April 3, 2025 17:56

eirbjo marked this pull request as ready for review April 4, 2025 04:46

openjdk bot added the rfr label Apr 4, 2025

openjdk bot removed the core-libs label Apr 4, 2025

dfuch reviewed Apr 4, 2025

View reviewed changes

eirbjo added 2 commits April 4, 2025 12:25

Do not assume that the loopback address is IPv4

18b449d

Use a fake host name since no actual FTP request will be attempted

049bb5f

dfuch approved these changes Apr 11, 2025

View reviewed changes

openjdk bot added the ready label Apr 11, 2025

openjdk bot added the integrated label Apr 11, 2025

openjdk bot closed this Apr 11, 2025

openjdk bot removed ready rfr labels Apr 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8353662: Add test for non-local file URL fallback to FTP #24418

8353662: Add test for non-local file URL fallback to FTP #24418

eirbjo commented Apr 3, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 3, 2025

openjdk bot commented Apr 3, 2025 •

edited

Loading

openjdk bot commented Apr 3, 2025

mlbridge bot commented Apr 4, 2025 •

edited

Loading

AlanBateman commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

dfuch Apr 4, 2025 •

edited

Loading

eirbjo Apr 4, 2025

dfuch Apr 4, 2025

eirbjo Apr 4, 2025

dfuch Apr 4, 2025

eirbjo Apr 11, 2025

dfuch Apr 11, 2025

eirbjo commented Apr 4, 2025

dfuch commented Apr 4, 2025 •

edited

Loading

eirbjo commented Apr 4, 2025 •

edited

Loading

eirbjo commented Apr 11, 2025

openjdk bot commented Apr 11, 2025

openjdk bot commented Apr 11, 2025

8353662: Add test for non-local file URL fallback to FTP #24418

8353662: Add test for non-local file URL fallback to FTP #24418

Conversation

eirbjo commented Apr 3, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 3, 2025

openjdk bot commented Apr 3, 2025 • edited Loading

openjdk bot commented Apr 3, 2025

mlbridge bot commented Apr 4, 2025 • edited Loading

Webrevs

AlanBateman commented Apr 4, 2025

openjdk bot commented Apr 4, 2025

dfuch Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

eirbjo Apr 4, 2025

Choose a reason for hiding this comment

dfuch Apr 4, 2025

Choose a reason for hiding this comment

eirbjo Apr 4, 2025

Choose a reason for hiding this comment

dfuch Apr 4, 2025

Choose a reason for hiding this comment

eirbjo Apr 11, 2025

Choose a reason for hiding this comment

dfuch Apr 11, 2025

Choose a reason for hiding this comment

eirbjo commented Apr 4, 2025

dfuch commented Apr 4, 2025 • edited Loading

eirbjo commented Apr 4, 2025 • edited Loading

eirbjo commented Apr 11, 2025

openjdk bot commented Apr 11, 2025

openjdk bot commented Apr 11, 2025

eirbjo commented Apr 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 3, 2025 •

edited

Loading

mlbridge bot commented Apr 4, 2025 •

edited

Loading

dfuch Apr 4, 2025 •

edited

Loading

dfuch commented Apr 4, 2025 •

edited

Loading

eirbjo commented Apr 4, 2025 •

edited

Loading