Skip to content

Update GRPC #6808

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

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

Update GRPC #6808

wants to merge 7 commits into from

Conversation

alanprot
Copy link
Member

@alanprot alanprot commented Jun 10, 2025

What this PR does:

This PR attempts to update the Go-GRPC library, which has been pinned for a while. The newer version of Go-GRPC reuses unmarshalling wire buffers internally, which can potentially race with our own buffer reuse logic, leading to message corruption.

A test was added to demonstrate this issue in the Push and QueryStream methods.

Problem background:

In Cortex, we use yolostring when unmarshalling bytes from the wire into proto structs (see: Cortex yolostring). Yolostring creates a string from the wire buffer’s bytes without copying them, effectively reusing the original bytes to reduce memory allocations.

Prior to Go-GRPC v1.65.0, wire buffers were created and used within a single request, so reusing those bytes via yolostring was safe.

However, starting with Go-GRPC v1.66.0, the wire buffers are now pooled and reused across multiple requests. The bytes are returned to the pool immediately after the Unmarshal method is called (reference and grpc/grpc-go#6619). This means that the bytes might be overwritten before the Cortex code finishes using them, leading to potential message corruption.

In this PR, we are creating a new Proto Codec that is not freeing the buffer used to unmarshall the message back to the pool: See this in comparison to the go-grpc code here - in practical terms, we are by default falling back to the behavior before v1.66.0. However, we can also now change the proto message to implement the ReleasableMessage interface and, if done, the message will now have an extra Free() that can be called to release the buffer back to the pool when is safe to do so -> so far, only the WriteRequest message is implementing this as this is the one that has a huge TPS with a very short lived lifecycle. In the future, we can add this capabilities to other proto messages.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • [NA] Documentation added
  • [NA] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@alanprot alanprot force-pushed the update-grpc branch 5 times, most recently from 265068a to ec132e6 Compare June 10, 2025 21:37
@alanprot alanprot changed the title Upadte GRPC => WIP Upadte GRPC Jun 11, 2025
@alanprot alanprot marked this pull request as ready for review June 11, 2025 17:09
@dosubot dosubot bot added dependencies Pull requests that update a dependency file go Pull requests that update Go code labels Jun 11, 2025
@alanprot alanprot requested review from danielblando, yeya24 and SungJin1212 and removed request for danielblando June 11, 2025 17:12
@alanprot alanprot changed the title Upadte GRPC Update GRPC Jun 11, 2025
Copy link
Member

@friedrichg friedrichg left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 11, 2025
Copy link
Contributor

@danielblando danielblando left a comment

Choose a reason for hiding this comment

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

LGTM

@SungJin1212
Copy link
Member

LGTM

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks. Great work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file go Pull requests that update Go code lgtm This PR has been approved by a maintainer size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants