Skip to content
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 sending compound messages corruption when including messages larger than 64KB #260

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pracucci
Copy link

@pracucci pracucci commented Mar 11, 2022

Cortex uses memberlist with a custom transport implementation, based on TCP protocol. We have a use case to send large messages, sometimes bigger than 64KB.

When memberlist implementation builds a compound message, it doesn't take in account the message length. Since in the compound message the length of an inner message is stored as a uint16, the compound message gets corrupted if contains an inner message larger than 64KB.

To solve this issue, I propose to not include messages larger than 64KB in a compound message.

Notes:

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 11, 2022

CLA assistant check
All committers have signed the CLA.

@alvinlin123
Copy link

@markan I wondering if you can take a look at this PR? I thought you might be a good candidate because you looked at the related PR: #239

Thanks in advance :)

@alvinlin123
Copy link

Sorry for the constant nudge, but just wondering if this PR looks good and can be merged soon?

@alvinlin123
Copy link

@markan any chance you would be able to take a look at this PR soon?

Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, sorry for the delayed review.
This looks good; one minor suggestion for clarity.

Comment on lines +171 to +185
r, w := 0, 0
for r < len(msgs) {
if len(msgs[r]) <= maxMsgLength {
// Keep it.
msgs[w] = msgs[r]
r++
w++
continue
}

// This message is a large one, so we send it alone.
bufs = append(bufs, bytes.NewBuffer(msgs[r]))
r++
}
msgs = msgs[:w]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this instead?

Suggested change
r, w := 0, 0
for r < len(msgs) {
if len(msgs[r]) <= maxMsgLength {
// Keep it.
msgs[w] = msgs[r]
r++
w++
continue
}
// This message is a large one, so we send it alone.
bufs = append(bufs, bytes.NewBuffer(msgs[r]))
r++
}
msgs = msgs[:w]
for r, b := range msgs {
if len(b) <= maxMsgLength {
// Keep it.
msgs[w] = b
w++
} else {
// This message is a large one, so we send it alone.
bufs = append(bufs, bytes.NewBuffer(b))
}
}
msgs = msgs[:w]

Should be logically equivalent, but removes the need for the continue and captures that we always advance r each step, so it's instantly obvious that the loop completes and looks at each input exactly once.

Or maybe this: (adapted from https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating)

Suggested change
r, w := 0, 0
for r < len(msgs) {
if len(msgs[r]) <= maxMsgLength {
// Keep it.
msgs[w] = msgs[r]
r++
w++
continue
}
// This message is a large one, so we send it alone.
bufs = append(bufs, bytes.NewBuffer(msgs[r]))
r++
}
msgs = msgs[:w]
w := a[:0]
for _, b := range msgs {
if len(b) <= maxMsgLength {
w = append(w, b)
} else {
// This message is a large one, so we send it alone.
bufs = append(bufs, bytes.NewBuffer(b))
}
}
msgs = w

Copy link

@Amier3 Amier3 Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alvinlin123 or @pracucci ,

Are you still able to follow up on these requested changes?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let me try to see if I can get a hold of @pracucci :-)

@alvinlin123
Copy link

No worries @markan, thank you so much for taking a look!

@jeromeinsf
Copy link

@pracucci Do you think you could take a look?

@alvinlin123
Copy link

@Amier3 @markan I have addressed the PR review comments in separate PR: #262

Please let me now if #262 looks ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants