search

MatrixAI / js-quic

QUIC Networking for TypeScript & JavaScript
https://matrixai.github.io/js-quic/
Apache License 2.0
13 stars 1 forks source link

`QUICStream` recreated after graceful destroy #66

Closed tegefaulkes closed 11 months ago

tegefaulkes commented 1 year ago

Specification

Sorry but I don't have many details here. I'll have to reproduce it in a test here.

In Polykey during one of the tests where a single RPC client call was made and ends gracefully. I noticed that a 2nd stream was created and passed to the RPCServer to be handled but it never reached the handler stage. This caused an issue where the RPCServer.stop() just hung since the stream never ended.

We've seen this problem before where the quiche stream state is not fully cleaned up after the stream has ended. In this case the stream still exists on the iterator and is re-created as a QUICStream. We need to double check if this is still a problem and make sure the streams are properly cleaned up in quiche.

To properly clean up a stream in quiche you need to attempt a write that throws if its a writable. or attempt a read that throws if it is a readable. If we don't do this the state wont transition.

Minor detail, but the stream was re-created on the client connection but I didn't see it on the server connection side.

Additional context

Tasks

  1. Check if the quiche stream state has been cleaned up properly on the client connection side for a gracefully ended stream.
  2. apply a fix if needed.
CMCDragonkai commented 1 year ago

I wonder if there's a check we can do when a stream being recreated like the new stream ID is below a counter. We have one-sided counter but we could also maintain the other sides counter so we know exactly when this occurs.

CMCDragonkai commented 1 year ago

Is this still a problem and should it be considered part of the stability issues?

tegefaulkes commented 1 year ago

I'll have to double check if this is still a problem.

CMCDragonkai commented 11 months ago

I think we should have an exception that throws if the same stream-id is re-used. Also stream ID is always incremented.

tegefaulkes commented 11 months ago

I had a look over it and added 3 tests. I don't see this problem happening currently and if it does with further changes the new test should catch it. I'm going to close this issue with the new commit. It doesn't warrant a PR.