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

Prevent reusing stream ids #52

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Conversation

stephenc-pace
Copy link
Collaborator

When we receive a StreamReset frame (for eg; when a client terminates a stream on timeout), we currently cleanup the receive stream but leave the send stream.
Stream objects in nameko-grpc are just wrappers for a single underlying http/2 stream

        request_stream = SendStream(stream_id)
        response_stream = ReceiveStream(stream_id)
        self.receive_streams[stream_id] = response_stream
        self.send_streams[stream_id] = request_stream

So when we don't cleanup both of these objects on stream close we can attempt to send data again on the SendStream object, which will either attempt to send data on a closed stream StreamClosedError (if h2 hasn't discared its reference to the underyling stream yet, or if it has, h2 will create a new stream object with the previous stream_id, resulting in the StreamIDTooLowError error when we attempt to send data.

This PR fixes this by ensuring the SendStream object is removed from our references when we receive a StreamResetframe.

"""
log.debug("stream reset, stream %s", event.stream_id)
receive_stream = self.receive_streams.pop(event.stream_id, None)
if receive_stream:
receive_stream.close()
send_stream = self.send_streams.pop(event.stream_id, None)
if send_stream:
send_stream.close()
Copy link
Collaborator Author

@stephenc-pace stephenc-pace Apr 3, 2023

Choose a reason for hiding this comment

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

Enusre our reference to the stream is closed.

@stephenc-pace
Copy link
Collaborator Author

@mattbennett Hope this makes sense. I need to add tests still to the PR (struggling with that part).

@stephenc-pace
Copy link
Collaborator Author

@mattbennett
I haven't added tests as it's quite challenging to do so.
We've had this in production for a number of months and it's resolved the issue identified without causing further problems so confident in it.

Copy link
Member

@mattbennett mattbennett left a comment

Choose a reason for hiding this comment

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

This looks like such a numbskull bug when it's on its own in a PR like this. Thanks for finding it though!

@timbu timbu merged commit 2f41946 into nameko:master Sep 21, 2023
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.

3 participants