RFR: 8358688: HttpClient: Simplify file streaming in RequestPublishers.FilePublisher

[Volkan Yazici]

On Thu, 5 Jun 2025 15:19:09 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 269:
>> 
>>> 267:                 // The old code was using `FileInputStream::new`, which throws `FNFE` if file doesn't exist.
>>> 268:                 // Preserving that behaviour after migrating to `Files::newInputStream`:
>>> 269:                 t = new FileNotFoundException(path + " (No such file or directory)");
>> 
>> Hello Volkan, just some initial comments. I think this `Files.isRegularFile(...)` check and the `Files.newInputStream(...)` call should be done in `RequestPublishers.create(...)` instead of here in subscribe. That way the checks happen when the `BodyPublishers.ofFile(...)` gets invoked instead of at subscription time. That then means that this `FilePublisher` will continue to accept a `InputStream` in its constructor, created out of a successful call to `Files.newInputStream()`. 
>> 
>> Moving these checks and call into `RequestPublishers.create(...)` will also mean that we won't need the code comments explaining why a `FileNotFoundException` gets thrown, because both `BodyPublishers.ofFile(...)` and the (internal) `RequestPublishers.create(...)` are already specified to throw `FileNotFoundException`
>
> Good idea. FWIW, and ulness I'm mistaken, I believe the old code was not always throwing `FileNotFoundException` - but might have thrown `NoSuchFileException` if  using  a non-default filesystem. If `FileNotFoundException` is specified and `NoSuchFileException` is not then that's probably a good change of behaviour.
> 
> Could be worth a release note (if that's the case and the behaviour changed) - but probably not a CSR.

Placing `isRegularFile()` check to `create()` indeed was my initial attempt too. Though that resulted in a behavior change: Earlier the regular file check was performed by `FIS::new` at subscription, hence, `NSFE` (yes, `FIS::new` throws `NSFE` when passed a not-regular file, e.g., a directory) was thrown at `subscribe()`. Moving this check from `subscribe()` to `create()` breaks the `RelayingPublishers` test, since failure gets moved from subscription-time to [reactive pipeline] assembly-time.

Would you prefer me to

1. keep things as is,
2. move `isRegularFile()` to `create()`, or
3. move `isRegularFile()` to `create()` and file a CSR?

Note that I don't think we can avoid the `NSFE`-to-`FNFE` translation in `subscribe()`, since, whatever earlier check we do before `Files::newIS`, file might get changed afterwards, and `NSFE` can still be thrown at the `Files::newIS` invocation.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25662#discussion_r2131681143