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

Improve gRPC cancellation #2051

Merged
merged 7 commits into from Jul 12, 2018
Merged

Improve gRPC cancellation #2051

merged 7 commits into from Jul 12, 2018

Conversation

adleong
Copy link
Member

@adleong adleong commented Jul 9, 2018

Fixes #1906

There is no good method for the consumer of a gRPC stream to signal that they are no longer interested in reading from the stream and that the producer should stop producing. In particular, for the io.l5d.mesh Namerd interface, this means that when Linkerd receives a gRPC stream from Namerd for a name, it will continue to receive updates for that name forever, even if it becomes no longer observed. This can result in a memory leak over time.

We update the gRPC stream API to add a reset method which the consumer can use to signal cancellation. We update the gRPC client and server dispatches to connect the gRPC resets to the H2 stream cancellation mechanism added in #1934. Much of the state management in DecodingStream is no longer necessary since we are simply passing cancellations through to the underlying H2 stream.

We also update the io.l5d.mesh Namerd client to cancel the stream when the corresponding Activity is no longer observed.

This was tested by using the script from #1906 and verifying that memory usage was stable.

Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Signed-off-by: Alex Leong <alex@buoyant.io>
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

We did an IRL review of this. This looks great to me!

Copy link
Contributor

@dadjeibaah dadjeibaah left a comment

Choose a reason for hiding this comment

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

This changes LGTM! I would need to spend a little bit more time in this code base to get more insight.

@@ -30,7 +31,7 @@ private[runtime] trait DecodingStream[T] extends Stream[T] {
* This state holds a `Releaser`, which manages when HTTP/2 frames
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still considered a state of the stream or is it now just functioning as a buffer? Maybe the comments need to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's still state and it still holds a releaser, even if it's less state than we were storing before.

@adleong adleong merged commit 9e63337 into master Jul 12, 2018
@adleong adleong deleted the alex/grpc-cancel branch July 12, 2018 21:06
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.

None yet

3 participants