On Thu, 5 Jun 2025 13:59:47 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Simplifies content streaming for `HttpRequest.BodyPublishers::ofFile(Path)`, >> which delegates to `RequestPublishers.FilePublisher::create(Path)`. This >> effectively replaces all usages of `FileInputStream::new` in `HttpClient` >> with `Files::newInputStream(Path)`. See the JBS ticket description for the >> full story. >> >> Also removing certain test files which were rendered redundant after >> `SecurityManager` removal. We do this in this PR, since tests were added due >> to touched lines. >> >> `tier1,2` results on b5f3c8dc6f4 are attached to the ticket. > > 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25662#discussion_r2129108800
