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.
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
Baggage support #4563
Baggage support #4563
Changes from all commits
1d46ff2
951e4cb
d14fe44
55c9880
0d626b2
87d8174
f49cb62
700f022
c0f74d8
d6672cc
d98648b
7ffc64a
4880dea
a4b90c7
b2ff71c
d1a2be3
49aaa7a
d514bed
32e91a7
8b9b917
de79a4e
dae0866
089353f
ac0f133
1ed2206
0b753fd
489a81e
052e693
6359a79
78c87d5
3719d66
54d926d
098ac33
2809fe4
511fe14
7109626
1f3ebc0
9f1bfac
2e6afab
cadc687
97ccd23
1249935
c27f768
58d1b7f
f898c98
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
While this code looks mostly correct, there's an edge case in
buf.subarray(0, this._config.baggageMaxBytes)
where we could theoretically split a multi-byte unicode character and end up with invalid UTF-8 bytes. I imagine if this happens thebuf.toString('utf8')
below would fail.BUT, I don't think this will actually ever happen because we should never get multi-byte characters here, since we should have encoded all of them already with
encodeURIComponent(key)
above.So now I'm thinking, if all characters in the string are guaranteed to be 1-byte ASCII characters after encoding, do we need even need
Bugger.from(baggage)
andbuf.toString('utf8')
? Or can we use the string's length directly without converting to UTF-8 bytes and back?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 a suggestion for my comment above, I think you can keep a count of bytes like the way you already keep a count of items, and follow similar logic. Something like
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.
Can we add non-ASCII characters like
é
or我
or🐶
to ensure those are encoded correctly, too?(I thought I added this comment before, but I don't see it. Sorry if it's a duplicate.)
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.
oops merged before I saw your comments, will create a separate PR to address your comments