search

nameko / nameko-grpc

GRPC Extensions for Nameko
Apache License 2.0
59 stars 12 forks source link

Prevent reusing stream ids #52

Closed stephenc-pace closed 1 year ago

stephenc-pace commented 1 year ago

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.

stephenc-pace commented 1 year ago

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

stephenc-pace commented 1 year ago

@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.