Conversation
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks for doing this @vonzshik.
When I thought about how to do it, I imagined a more explicit/high-level API where we would call "StartNewFrame" on the buffer, providing the message code and length, rather than inferring that a new frame is starting from the setting of ReadPosition (when _inFrame is false). It seems like this might considerably simplify things (I'm still not clear on the exact logic of _bufferReset, ResetFrame, etc.).
What do you think?
| await Authenticate(username, timeout, async, cancellationToken); | ||
|
|
||
| #if DEBUG | ||
| ReadBuffer.EnableFrameTracking = true; |
There was a problem hiding this comment.
Any particular reason this shouldn't always be on (in DEBUG)? We could also use it to catch bugs in the Startup message and in authentication...
| { | ||
| await Authenticate(username, timeout, async, cancellationToken); | ||
|
|
||
| #if DEBUG |
There was a problem hiding this comment.
Rather than wrapping each call with #if DEBUG, the methods themselves can have [Conditional("DEBUG")]. This automatically does what we want, and makes the call sites much nicer.
| } | ||
| else | ||
| { | ||
| _currentFrame.LengthLeft -= positionDelta; |
There was a problem hiding this comment.
nit: maybe better to have Position (in the frame) - just like the the buffer's ReadPosition - which gets incremented, rather than LengthLeft which gets decremented.
| { | ||
| Debug.Assert(positionDelta == 1, "Protocol failure"); | ||
| Debug.Assert(ReadBytesLeft >= 5); | ||
| var frameCode = Buffer[_readPosition]; |
4 participants
Related to #3775
Testing whether it breaks something...