RFR: 8136895: Writer not closed with disk full error, file resource leaked [v2]

[Daniel Jeliński]

On Tue, 18 Apr 2023 19:51:03 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> In`StreamEncoder::implClose`, move `flushLeftoverChar()` inside the `try` block and the closing of the underlying stream inside the `finally` block.
>
> Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8136895: Do not swallow exceptions in StreamEncoder::implFlush; add test for OutputStreamWriter; rename test for Channels::newWriter"

If `flushLeftoverChar` throws, `writeBytes` will not be called and any characters left in `bb` will be lost. This should probably be fixed, but given that it's preexisting, it may be deferred to a separate issue.
Encoder.flush won't be called either; the method has a non-trivial implementation for charset ISO-2022-JP, for example. I'm not sure if there's anything we should do about that.

Also, I checked why `out.flush` was added; found [this mail](https://mail.openjdk.org/pipermail/nio-dev/2019-March/005974.html) stating that `StreamEncoderClose` test failed without that line. I checked the test, and it appears to assume that if StreamEncoder catches an exception when closing the underlying stream, it should be closed again. While this might have made sense back when it was implemented, today try-with-resources gives each `Closeable` only one chance to clean up. I still think we should just use `try (ch; out)` and remove the explicit `flush` & `close` calls.

Also, StreamEncoderTest is linked to [JDK-5005426](https://bugs.openjdk.org/browse/JDK-5005426); the bug describes exactly the issue mentioned in my first point above - `flushLeftoverChar` threw an exception and data buffered by `StreamEncoder` was discarded. We could implement a better test for that (e.g. write "Test\ud83c", then close, then check if "Test" was written).

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

PR Comment: https://git.openjdk.org/jdk/pull/13503#issuecomment-1522914268