On Thu, 5 Jun 2025 15:19:09 GMT, Daniel Fuchs <dfu...@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
