Update NamedPipeStreamReader to handle multiple sequential messages#341
Update NamedPipeStreamReader to handle multiple sequential messages#341jamill wants to merge 5 commits intomicrosoft:masterfrom
Conversation
The NamedPipe protocol did not handle the case where multiple messages were sent sequentially without waiting for a response from the other side. This changes the NamedPipeStreamReader to inspect all the bytes and handle cases where there are multiple messages sent.
Previously, the NamedPipeStreamReader made the assumption that only a single message would be sent at a time. This assumption simplified its logic, and it assumed that the end of the message would be at the end of the buffer. If the buffer contained multiple messages, it might report multiple messages as a single message. This change updates the NamedPipeStreamReader to handle a message terminated at any point.
| // We have read all the bytes in the buffer - | ||
| // set the start of the next message to be | ||
| // the following index. | ||
| this.nextMessageChunkStartIndex = i; |
There was a problem hiding this comment.
What if the message (or the processing of multiple messages) is long enough that we've filled up this.buffer? Does this.nextMessageChunkStartIndex need to get set back to 0?
(I'm not sure what the impact of this change would be on the other index related checks)
There was a problem hiding this comment.
Yes - nextMessageChunkStartIndex gets reset on the Read()
There was a problem hiding this comment.
Ok thanks, I think I was getting messageChunkStartIndex and nextMessageChunkStartIndex confused when reading through the while loop.
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
This is currently very complex, and I worry that it will be difficult to maintain (or even to ensure it's bug-free now). Please take a look and see if we can simplify it down.
| /// | ||
| /// This format was chosen so that | ||
| /// 1) A reasonable range of values can be transmitted across the pipe, | ||
| /// including including null and bytes that represent newline characters, |
There was a problem hiding this comment.
Typo: "including including"
| /// The number of bytes that have been read from the stream and into | ||
| /// the buffer. | ||
| /// </summary> | ||
| private int numBytesReadIntoBuffer; |
There was a problem hiding this comment.
This variable looks like it represents the number of bytes left to consume, but it actually just represents the number of bytes that we got back in the last read. I think this will be confusing to maintain, and it would be nice to eliminate this variable if we can. Still reading...
There was a problem hiding this comment.
Yea I think we should either treat buffer as a circular buffer, and track a start index and number of unread bytes, or else just make the buffer big enough that it won't need to wrap and simplify this down. The logic is currently very hard to follow.
There was a problem hiding this comment.
Or yet another option would be to get rid of buffer altogether, and only read one byte at a time until you find the message terminator. How much would that affect perf? If it's not a significant hit, that will eliminate most of the complexity here.
There was a problem hiding this comment.
else just make the buffer big enough that it won't need to wrap and simplify this down
The message can contain user entered text (git commands) and arbitrary content (Modified Paths entries) - I don't think we can put a cap on the message buffer.
another option would be to get rid of buffer altogether,
I have not profiled it, but the msdn documentation includes a note about how inefficient this approach is Notes to Inheritors.
There was a problem hiding this comment.
There's no question that reading one byte at a time will be slower, but how much will that impact our actual perf numbers on git commands?
If that doesn't pan out because the perf is bad, then let's chat about options for making this solution simpler.
| } | ||
|
|
||
| [Test] | ||
| [Description("Verify that we can transmit message multiple messages in the same buffer")] |
There was a problem hiding this comment.
Please drop the description attributes. They tend to get stale, and the method name should just be the description anyway.
| { | ||
| string chunkedMessage = new string('T', BufferSize + (BufferSize / 2)); | ||
|
|
||
| // Verify that we corrently handle messages |
|
This was handled in #343 |
4 participants
Originally, the NamedPipeStreamReader assumed that only a single message would be sent over the pipe at a time. This allowed for some simplifications to the
NamedPipeStreamReader. However, this assumption does not hold for and results in incorrect behavior when multiple messages are sent (see #333).This series of commits adds test cases demonstrating this issue and updates the NamedPipeStreamReader to handle multiple sequential messages on the NamedPipe stream.
Fixes #333