8329829: HttpClient: Add a BodyPublishers.ofFileChannel method

[vy]

Adds a new ofFileChannel(FileChannel channel, long offset, long length) method to java.net.HttpRequest.BodyPublishers to provide an HttpClient publisher to upload a certain region of a file. The new publisher does not modify the state of the passed FileChannel, streams the file channel bytes as it publishes (i.e., avoids reading the entire file into the memory), and can be leveraged to implement sliced uploads. As noted in the Javadoc:

The file channel will not be closed upon completion. The caller is
expected to manage the life cycle of the channel, and close it
appropriately when not needed anymore.

Implementation notes

• FileChannel is preferred over {Readable,Seekable}ByteChannel, since the latter does not provide a positional read without modifying the state of the FileChannel, which is necessary to use a single FileChannel instance to implement sliced uploads.
• ofFileChannel(FileChannel,long,long) is preferred over ofPath(Path,long,long) to avoid overloading the maximum file descriptor limit of the platform.

---

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-8329829: HttpClient: Add a BodyPublishers.ofFileChannel method (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26155

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vyazici! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@vy The following label will be automatically applied to this pull request:

• net

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

[mlbridge]

Webrevs

• 04: Full - Incremental (99503c0b)
• 03: Full - Incremental (da698406)
• 02: Full - Incremental (3af61c5f)
• 01: Full - Incremental (0eda56f1)
• 00: Full (bdeb999b)

[jaikiran]

I think some of the text in this javadoc would need changes/clarifications. I haven't added any review comments for it yet and will come to it later once we have settled on the rest of the review.

[jaikiran]

Thank you for the updates - the implementation changes look good to me. I'll focus on the test and the API javadoc text next.

[jaikiran]

I think the application should be receiving a IOException (both for send() and sendAsync()) instead of a UncheckedIOException.

[vy]

The IOE thrown is wrapped by an UncheckedIOE in FileChannelIterator::next, which overrides Iterator::next and that does not allow a throws in the next() footprint. Would you mind elaborating on your remark, please?

[jaikiran]

Hello Volkan, the specification of HttpClient.send(...) (and sendAsync()) is that it throws a checked IOException. So any other exceptions that we throw internally (like this one) need to be converted to an IOException when it reaches the application code.

We have code in HttpClientImpl.send(...) which currently does instanceof checks against these exceptions and converts them to an IOException. I'm guessing your test is currently passing, which suggests to me that sendAsync() is propagating a UncheckedIOException to the application code. Would it be possible to tweak the test a bit to replace that call of sendAsync() with a send() (even if those tweaked changes cannot be pushed to this PR) and see what gets propagated? I suspect it would be IOException.

[vy]

I've confirmed that updating the test to use send() results in IOE getting received. I have not updated the PR in this direction, since it adds more boilerplate, and I read your lines as, not a change, but a research request – let me know if you meant otherwise.

the specification of HttpClient.send(...) (and sendAsync()) is that it throws a checked IOException

HttpClient::send has the following Javadoc:

     * @throws IOException if an I/O error occurs when sending or receiving, or
     *         the client has {@linkplain ##closing shut down}
     * @throws InterruptedException ...
     * @throws IllegalArgumentException ...

whereas ::sendAsync Javadoc has no details regarding I/O failures. Do you think the fact that sendAsync() does not have a to-IOException-translation is an oversight that should be improved, or an intentional slack for the implementation?

[jaikiran]

Hello Volkan,

whereas ::sendAsync Javadoc has no details regarding I/O failures.

The sendAsync() has the same specification about IOException as that of send(). It says:

* <p> The returned completable future completes exceptionally with:
* <ul>
* <li>{@link IOException} - if an I/O error occurs when sending or receiving,
*      or the client has {@linkplain ##closing shut down}.</li>
* </ul>

Do you think the fact that sendAsync() does not have a to-IOException-translation is an oversight that should be improved, or an intentional slack for the implementation?

My expectation is that both send() and sendAsync() should propagate the same exception to the application (of course for sendAsync() it would be propagated as a cause in the ExecutionException). So this is a surprise to me that we aren't currently doing that. We'll need inputs from Daniel (@dfuch) and Michael (@Michael-Mc-Mahon) whether this is intentional or an oversight.

I have not updated the PR in this direction, since it adds more boilerplate, and I read your lines as, not a change, but a research request

Yes, that's OK. Having said that, if it's easy to add a test which uses send() and causes an IOException (due to the FileChannel being closed mid way, for example), then it would be a good thing since that will then bring in the necessary coverage for the exception propagation for that method too.