-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(codec): Properly decode partial DATA frames #83
fix(codec): Properly decode partial DATA frames #83
Conversation
…we continue reading
@@ -0,0 +1,29 @@ | |||
// Copyright 2015 gRPC authors. | |||
// | |||
// Licensed under the Apache License, Version 2.0 (the "License"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not accurate, I think. It's Apache OR MIT dual-licensed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an addendum, the right thing to do if license headers are desired is to use SPDX format ones, otherwise the LICENSE in the repo works just fine.
@cpcloud so if I understand correct the issue is that we may not have the initial 5 bytes from the buffer handed to us from hyper? Thus the |
@LucioFranco No, the issue is that we'll receive the header + a partial body. For example:
We need to ensure that |
Ok so after our discussion on discord I am happy with this solution. I hate to ask this but would it be possible to try and make this change with the smallest footprint possible which I think is just the second if statement in readbody. And instead of introducing a new example service can we add a test, there should already be tests for the codec. In the mock body simulate that the frames are split. |
and thank you so much for figuring this out! It is super helpful, I'd like to get this change in before the next release this week. Let me know if you need anything. |
@LucioFranco Looks like the |
I'll add a fix in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you so much for digging into this!
Yeah! Thanks for reviewing + merging! |
Closes #76
TODO:
Motivation
Reproduced from #76:
Solution
The solution that I've implemented here is to check that
self.buf
--the bufferthat holds the bytes for decoding--has enough data to decode both the tonic
header and the message. If the check returns
false
, wait for more data,otherwise decode the bytes