Why doesn't Channels.newChannel(OutputStream) flush?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Why doesn't Channels.newChannel(OutputStream) flush?

Charles Oliver Nutter-4
While debugging some subprocess logic in JRuby today I came to the
realization that the WritableByteChannel returned by
Channels.newChannel(OutputStream) is "broken".

https://github.com/jruby/jruby/pull/6649/commits/5a6912caf3adb70fb4c210c73ae02fbf16f404d9

The issue arises when the provided stream is itself buffered, as is
the case for Process.getOutputStream. Because neither Channel nor
WritableByteChannel provide a way to flush (as Channels are expected
to be "raw" unbuffered IO), data written to the wrapper will not
propagate to the the other end of the stream until the intermediate
buffer has been filled. A flush can only be performed if you have
direct access to the underlying stream, which is generally impossible
if using this wrapper to interact with a Channel-related API.

I would propose that the default OutputStream Channel wrapper should
flush() after every non-zero write (or potentially after every write,
if the underlying stream is not altogether honest about how many bytes
were written).

Can someone provide a counter example for why the wrapper should *not*
flush (current behavior)? What would be harmed by making this change?
I can come up with nothing.

 Additional reasons to flush:

* Early propagation of IO errors due to the underlying stream no
longer being flushable.
* More responsive, possibly reducing IO stutter for APIs using the wrapper.

That first one could be seen as a negative but I can only see it as a
positive (raise the error at the time of the bad write, not once the
buffer is filled many writes later).

- Charlie
Reply | Threaded
Open this post in threaded view
|

Re: Why doesn't Channels.newChannel(OutputStream) flush?

Brian Burkhalter-2
Hello,

This does not sound unreasonable to me, although I do not know the historical context of the design decisions involved, on which I expect that Alan Bateman will comment tomorrow.

As to the implementation, it might be just to modify java.nio.channels.Channels$WritableByteChannelImpl.write() to flush the wrapped stream in the sync block after the write loop.

Brian

On Apr 6, 2021, at 12:30 PM, Charles Oliver Nutter <[hidden email]<mailto:[hidden email]>> wrote:

While debugging some subprocess logic in JRuby today I came to the
realization that the WritableByteChannel returned by
Channels.newChannel(OutputStream) is "broken".

https://github.com/jruby/jruby/pull/6649/commits/5a6912caf3adb70fb4c210c73ae02fbf16f404d9

The issue arises when the provided stream is itself buffered, as is
the case for Process.getOutputStream. Because neither Channel nor
WritableByteChannel provide a way to flush (as Channels are expected
to be "raw" unbuffered IO), data written to the wrapper will not
propagate to the the other end of the stream until the intermediate
buffer has been filled. A flush can only be performed if you have
direct access to the underlying stream, which is generally impossible
if using this wrapper to interact with a Channel-related API.

I would propose that the default OutputStream Channel wrapper should
flush() after every non-zero write (or potentially after every write,
if the underlying stream is not altogether honest about how many bytes
were written).

Can someone provide a counter example for why the wrapper should *not*
flush (current behavior)? What would be harmed by making this change?
I can come up with nothing.

Additional reasons to flush:

* Early propagation of IO errors due to the underlying stream no
longer being flushable.
* More responsive, possibly reducing IO stutter for APIs using the wrapper.

That first one could be seen as a negative but I can only see it as a
positive (raise the error at the time of the bad write, not once the
buffer is filled many writes later).

Reply | Threaded
Open this post in threaded view
|

Re: Why doesn't Channels.newChannel(OutputStream) flush?

Charles Oliver Nutter-4
I agree, adding a simple flush basically solves this issue. I look
forward to hearing whether there's any good reason not to do so.

On Tue, Apr 6, 2021 at 3:38 PM Brian Burkhalter
<[hidden email]> wrote:

>
> Hello,
>
> This does not sound unreasonable to me, although I do not know the historical context of the design decisions involved, on which I expect that Alan Bateman will comment tomorrow.
>
> As to the implementation, it might be just to modify java.nio.channels.Channels$WritableByteChannelImpl.write() to flush the wrapped stream in the sync block after the write loop.
>
> Brian
>
> On Apr 6, 2021, at 12:30 PM, Charles Oliver Nutter <[hidden email]> wrote:
>
> While debugging some subprocess logic in JRuby today I came to the
> realization that the WritableByteChannel returned by
> Channels.newChannel(OutputStream) is "broken".
>
> https://github.com/jruby/jruby/pull/6649/commits/5a6912caf3adb70fb4c210c73ae02fbf16f404d9
>
> The issue arises when the provided stream is itself buffered, as is
> the case for Process.getOutputStream. Because neither Channel nor
> WritableByteChannel provide a way to flush (as Channels are expected
> to be "raw" unbuffered IO), data written to the wrapper will not
> propagate to the the other end of the stream until the intermediate
> buffer has been filled. A flush can only be performed if you have
> direct access to the underlying stream, which is generally impossible
> if using this wrapper to interact with a Channel-related API.
>
> I would propose that the default OutputStream Channel wrapper should
> flush() after every non-zero write (or potentially after every write,
> if the underlying stream is not altogether honest about how many bytes
> were written).
>
> Can someone provide a counter example for why the wrapper should *not*
> flush (current behavior)? What would be harmed by making this change?
> I can come up with nothing.
>
> Additional reasons to flush:
>
> * Early propagation of IO errors due to the underlying stream no
> longer being flushable.
> * More responsive, possibly reducing IO stutter for APIs using the wrapper.
>
> That first one could be seen as a negative but I can only see it as a
> positive (raise the error at the time of the bad write, not once the
> buffer is filled many writes later).
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Why doesn't Channels.newChannel(OutputStream) flush?

Alan Bateman
In reply to this post by Charles Oliver Nutter-4
On 06/04/2021 20:30, Charles Oliver Nutter wrote:

> While debugging some subprocess logic in JRuby today I came to the
> realization that the WritableByteChannel returned by
> Channels.newChannel(OutputStream) is "broken".
>
> https://github.com/jruby/jruby/pull/6649/commits/5a6912caf3adb70fb4c210c73ae02fbf16f404d9
>
> The issue arises when the provided stream is itself buffered, as is
> the case for Process.getOutputStream. Because neither Channel nor
> WritableByteChannel provide a way to flush (as Channels are expected
> to be "raw" unbuffered IO), data written to the wrapper will not
> propagate to the the other end of the stream until the intermediate
> buffer has been filled. A flush can only be performed if you have
> direct access to the underlying stream, which is generally impossible
> if using this wrapper to interact with a Channel-related API.
>
> I would propose that the default OutputStream Channel wrapper should
> flush() after every non-zero write (or potentially after every write,
> if the underlying stream is not altogether honest about how many bytes
> were written).
Channels.newChannel(OutputStream) dates from Java 1.4 and JSR-51 so I
don't think we can rush into changing it. If a writable channel is
created on an buffered stream then it will usually be up to the
underlying stream as to how much to buffer and when to flush when it
wraps another stream. So I think it will take a bit of exploration and
maybe there is a case for an overload that configures when if/when it
flushes. A PrintStream like autoFlush may or may not be enough.

-Alan.