Fix a few issues with ShellStream

[Rob-Hague]

I have not yet rebased this on the expectSize changes merged today, pending further discussion. So there are merge conflicts from the start. Just wanted to get it out there.

---

The main change is to replace the Queue with a byte[] and a couple of variables which index the start and end of the data. The remainder is mainly slightly more careful locking semantics.

One possibly contentious point: in fixing the Write behaviour (#301) I chose to remove the outgoing buffer and immediately send the data across the channel. Write(string) and WriteLine(string) were already doing this, and I felt it was better to change Write(byte[]) to match rather than changing the string methods. This is the reason for the number of deleted test classes for Write.

It also makes the definition of infinite timeout to consistently be -1 milliseconds instead of 0, as is standard in other .NET methods with a wait period. This was not documented previously.

The rest of the changes are hopefully fairly agreeable. Happy to clarify anything.

Here are the (allocation) numbers on 3bfac50a, which this is based on:

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
ShellStreamReadLine	865.1 ms	7.89 ms	6.99 ms	-	-	507.98 KB
ShellStreamExpect	864.8 ms	5.95 ms	5.57 ms	6000.0000	6000.0000	12565.52 KB

And here is after:

Method	Mean	Error	StdDev	Allocated
ShellStreamReadLine	866.6 ms	9.37 ms	8.77 ms	389.62 KB
ShellStreamExpect	851.1 ms	8.27 ms	7.74 ms	408.65 KB

(for reference, current develop d07827b6)

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
ShellStreamReadLine	866.3 ms	10.52 ms	9.32 ms	-	-	510.59 KB
ShellStreamExpect	864.7 ms	6.10 ms	5.70 ms	1000.0000	1000.0000	3321.11 KB

closes #63 closes #453 closes #672 closes #917 closes #180 closes #301 closes #365 (the previous implementation would read for the full timeout on each update, rather than just how long we have left) closes #714 closes #748 closes #302 closes #1320 closes #923 closes #1024

[WojciechNagorski]

It looks great! I'm patiently waiting for the merge with the develop branch

[WojciechNagorski]

Is it ready for review?

[Rob-Hague]

Yes, thanks

[WojciechNagorski]

Changing the behavior of the Write method is controversial, but we can try.

[Rob-Hague]

Yeah, I think either WriteLine and Write(string) needed to be buffered (which would require users to add a call to Flush), or Write(byte[]) needed to not buffer. I think changing Write(byte[]) to not buffer is the safest thing there.

edit: it could be flagged in the release notes as a behavioural breaking change

[jscarle]

I propose to do it the other way around. WriteLine could call Flush internally. And Write could buffer.

[jscarle]

Basically, my reasoning is that if I'm consuming ShellStream in my application, when I call WriteLine, I'm done. That's what I wanted to send. While using Write, maybe I have several different Writes to do before I'm done.

[Rob-Hague]

Yep that makes sense, but note that Write(string) will also have to flush internally to meet the same behaviour as before. It's only Write(byte[]) that wouldn't flush.

[jscarle]

Yep that makes sense, but note that Write(string) will also have to flush internally to meet the same behaviour as before. It's only Write(byte[]) that wouldn't flush.

I think that continues to make sense. If you're sending a full string, you probably are done as well. I suspect that people who are calling Write repeatedly will do so with byte[] and not strings.

[Rob-Hague]

Thanks. I will restore the write buffer shortly

[WojciechNagorski]

Okej so then we will release the new version

[jscarle]

@WojciechNagorski I'd like to submit my PR before you release if you don't mind.

[WojciechNagorski]

@jscarle Ok, no problem, just let me know which one.

[jscarle]

@jscarle Ok, no problem, just let me know which one.

1321