PipeStream returns 0 instead of blocking when running a command with…

[lucastheisen]

… intermittent output #12

[drieseng]

Disclaimer:
I haven't had time to think this through thoroughly, so it's very well possible that I'm talking nonsense :p

I propose the following changes:

• Modify PipeStream.Read(byte[] buffer, int offset, int count) to block until at least one byte of data is available, and not until count bytes are available.

• Modify PipeStream.Flush() to no longer unblock any blocking reads, and no longer cause any subsequent reads to also return immediately - even if no bytes are available at all - until a write is done.

  This would effectively make PipeStream.Flush() a noop.

• Add an internal PipeStream.EndOfStream() method that will signal to PipeStream that no more data will be written. Once this method is invoked, PipeStream.Read(byte[] buffer, int offset, int count) will no longer block if no data is available at all and will return zero in that condition. It's up to SshCommand to invoke the PipeStream.EndOfStream() when applicable (e.g. when the command has terminated).

For applications that consume SshCommand.OutputStream and SshCommand.ExtendedOutputStream, this means that they can now continue to read data as it becomes available while at the same time no longer wrongly consider the stream as closed.

For those applications that use SshCommand.Result and SshCommand.Error to read partial output, these changes could be considered breaking. We use a StreamReader (and more specifically StreamReader.ReadToEnd) to return stdout/stderr. Due to the fact that we flush the PipeStream upon reception of any bytes - and flushing currently causes any subsequent read to return immediately, as mentioned above - the StreamReader.ReadToEnd does not block until the command (more specially the channel) terminates, but instead returns all buffered text (that is not yet read).

After the changes described above, the SshCommand.Result and SshCommand.Error properties (or any code that reads the SshCommand.OutputStream and SshCommand.ExtendedOutputStream until the end of the stream) would block until the end of stream is reached (when the command terminates). Without any further changes, these properties would no longer allow intermediary output to be read.

The SshCommand.Result and SshCommand.Error properties can easily be modified by not using StreamReader.ReadToEnd(), and instead reading up to the number of bytes available in the PipeStream. Note that StreamReader applies a minimum buffer size of 128 characters. Therefore we will need to modify PipeStream.Read(byte[] buffer, int offset, int count) to only block until at least one byte of data is available. If not, StreamReader would block until at least 128 characters characters are available. This would again prevent these properties from returning intermediary output.

Applications that expect the SshCommand.OutputStream and SshCommand.ExtendedOutputStream to signal an end of stream when - at a given point in time - no more data is available to read, will consider the changes outlined above as breaking changes. These applications expect this end-of-stream situation well before the command has actually terminated, and while the command can still very well produce more output.

[lucastheisen]

For those applications that use SshCommand.Result and SshCommand.Error to read partial output, these changes could be considered breaking

I, personally, would not consider this breaking as it seems wrong to expect partial results from either of those properties (unless explicitly documented that way). I would expect either one to either (in this order):

1. read the PipeStream current contents at the moment of the call
2. return null
3. block until the command completes (though this seems like a silly thing for a property to do)

In either case, this leads to a real problem. If the entire output/error is actually being buffered on the command object, long running services like mine (or perhaps a tail -f to watch a log) will eventually run into memory issues (as the .Result and/or .Error properties will continue to grow unbounded. Perhaps, we should indicate ahead of time how the command will be used? Either we provide an out stream and err stream at command creation time, or the data gets buffered... What do you think?

Applications that expect the SshCommand.OutputStream and SshCommand.ExtendedOutputStream to signal an end of stream when - at a given point in time - no more data is available to read, will consider the changes outlined above as breaking changes. These applications expect this end-of-stream situation well before the command has actually terminated, and while the command can still very well produce more output.

Again, I would consider it wrong to expect the SshCommand to indicate in any way that it has reached the end of a stream when it has not actually done so. That is, unless the ssh protocol allows the closing of streams before the connection closes, in which case that information could just be propagated. Rather than waiting for the connection to close wait the ssh channels corresponding stream to close. But I dont think the protocol allows for that (not 100% sure).

All of that said, I understand the aversion to breaking changes, and you likely know best how to handle this. Other than the unbounded growth of the SshCommand object, I see no obvious problems with your suggested implementation.

[drieseng]

I have no aversion to breaking changes, I only wanted to explain why it could be considered a breaking change.

I will fix this once the 2016.0.0 is finalized.

[lucastheisen]

@drieseng , I saw you did recently release the 2016.0.0 version... Did you include a fix for this issue in it? Should i stop using my custom build?