Fixes argument truncation where long launch.json arguments were cut off at ~2KB, causing debugger launch failures.
#1529
Conversation
| stdout = _process.StandardOutput; | ||
| // Creating a new stream writer to set encoding to UTF-8 with UTF8Identifier as false. This prevents sending Byte Order Mask within the stream. | ||
| stdin = new StreamWriter(_process.StandardInput.BaseStream, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false), 1024 /* note: this is the default buffer size in the BCL */, leaveOpen: true); | ||
| stdin = new StreamWriter(_process.StandardInput.BaseStream, new UTF8Encoding(encoderShouldEmitUTF8Identifier: false), 65536 /* note: this is the default buffer size in the BCL */, leaveOpen: true); |
There was a problem hiding this comment.
I assume this comment is no longer true so it should be deleted: /* note: this is the default buffer size in the BCL */
| sslStream.AuthenticateAsClientAsync(tcpOptions.Hostname, certStore.Certificates, System.Security.Authentication.SslProtocols.None, false /* checkCertificateRevocation */).Wait(); | ||
| reader = new StreamReader(sslStream); | ||
| writer = new StreamWriter(sslStream); | ||
| reader = new StreamReader(sslStream, Encoding.UTF8, false, 65536); |
There was a problem hiding this comment.
This value 65536 is now mentioned in multiple locations in the code, I suggest that it should be a named constant instead, used everywhere. This helps the next reader understand that these values are the same intentionally, not by accident.
balint-bognar-goto
left a comment
balint-bognar-goto
left a comment
There was a problem hiding this comment.
As far as I understand the code (very little), the change looks OK to me.
|
First off, I want to thank you for taking the time to figure this out, and for taking the time to send this PR. Also, sorry it has taken us a while to respond. I am very glad you found something that seems to work, but I don't understand why it works -- the way things should work is that the size of the buffer only matters for performance reasons: MIEngine should write to the buffer until it hits the limit, at which point the write should block, then lldb-mi should read the bytes out of the buffer, which should then let MIEngine write the next set of bytes etc until the command is written. So, I don't have a good theory for why changing the buffer size fixes things. The only thought I had is maybe lldb-mi has a bug in the way it is reading from the buffer where it is giving up after a read of some size succeeds even if it lacks a trailing newline, but this is very much a guess. Did you understand why the fix works? BTW: Most folks are my team will be off for the holidays after this week, so if you reply, it might be a bit of time before we get back to you. |
|
@gregg-miskelly I confirmed via breakpoint that cmd is the full un-truncated string at _writer?.WriteLine(). The issue appears to be in lldb-mi's read logic—it seems to stop reading after receiving data up to the smaller buffer limit, even without a complete command. |
gregg-miskelly
left a comment
gregg-miskelly
left a comment
There was a problem hiding this comment.
Thanks again for taking the time to submit this. If you can make these tweaks, I will be happy to merge it, though it will be a little while probably when I am back from vacation.
… only - Declare STREAM_BUFFER_SIZE locally in PipeTransport.cs with comment about lldb-mi bug - Revert changes to StreamTransport.cs, TcpTransport.cs, and JDbg/TcpTransport.cs - These files don't need modification as they don't communicate with lldb-mi
|
@gregg-miskelly All changes addressed and pushed. Thanks for the review. |
|
@Subham-KRLX Thanks again for taking the time to send this out, and being so responsive! |
3 participants
Fixes microsoft/vscode-cpptools#14054
Increased PipeTransport buffer size to 64KB (from 1KB/2KB) to prevent long
launch.jsonarguments from being truncated with lldb-mi.