Fix field stop read in duplicate_protocol.go#3125
Merged
fishy merged 3 commits intoapache:masterfrom Apr 21, 2025
Merged
Conversation
When generated code reads a struct, it runs a `for` loop calling `ReadFieldBegin` at the top, but breaks if the field type ID is `thrift.STOP`. With TDuplicateToProtocol naively writing everything read, this results in extra writes, which breaks just about any protocol in the `DuplicateTo` struct field. The proposed fix is to simply add special handling for `thrift.STOP` to `ReadFieldBegin`. I'm no thrift expert, so I have no idea how other libraries handle this concern. Ideally, it seems like each protocol should understand and enforce the invariant that an attempt to call `WriteFieldBegin` with type ID 0 either isn't valid or is a misguided attempt to call `WriteFieldStop`.
KostenetskyiAndrii
approved these changes
Apr 18, 2025
fishy
reviewed
Apr 21, 2025
Member
fishy
left a comment
fishy
left a comment
There was a problem hiding this comment.
Can you please add a test (since this needs to use a struct, the test needs to be under lib/go/test) that would catch the bug?
Contributor
Author
I have written a test in the way that made sense to me. |
fishy
reviewed
Apr 21, 2025
Co-authored-by: Yuxuan 'fishy' Wang <fishywang@gmail.com>
fishy
approved these changes
Apr 21, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When generated code reads a struct, it runs a
forloop callingReadFieldBeginat the top, but breaks if the field type ID isthrift.STOP.With TDuplicateToProtocol naively performing an equivalent write for every read, this results in extra bytes, which breaks just about any protocol in the
DuplicateTostruct field.The proposed fix is to simply add special handling for
thrift.STOPtoReadFieldBegin.I'm no thrift expert, so I have no idea how other libraries handle this concern. Ideally, it seems like each protocol should understand and enforce the invariant that an attempt to call
WriteFieldBeginwith type ID 0 either isn't valid or is a misguided attempt to callWriteFieldStop, which could be performed instead, perhaps with the additional guardrail that a sequence of stops (with length > 1) never makes any sense.[skip ci]anywhere in the commit message to free up build resources.^^ hopefully we can agree this change is trivial, I very badly do not want to look at anyone's Jira, especially if it doesn't belong to my employer.