RFR: 8358688: HttpClient: Simplify file streaming in RequestPublishers.FilePublisher
Volkan Yazici
vyazici at openjdk.org
Fri Jun 6 07:46:56 UTC 2025
On Fri, 6 Jun 2025 07:38:21 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:
>> 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.
> I believe the old code was not always throwing `FileNotFoundException` - but might have thrown `NoSuchFileException` if using a non-default filesystem.
@dfuch, correct. If `Path` is associated with a file system that doesn't support `Path::toFile`<sup>1</sup>, earlier users would get `Files::newIS` exceptions (which were indeed undocumented), whereas others would get `FIS::new` exceptions.
<sup>1</sup> Allow me to note that I intentionally avoid using "default file system" term, since, IMHO, that distracts us from the problem: file systems that don't support `Path::toFile`. The "default file system" is required to support that feature, but there can be other file systems that support that too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25662#discussion_r2131690023
More information about the net-dev
mailing list