fix ending `QUICConnection` with `force: false`

[tegefaulkes]

Description

While working on https://github.com/MatrixAI/Polykey/pull/609 I found the following problems.

1. When ending a connection with force: false any currently running streams will end with an error. The expected behaviour is that they finish gracefully.
2. If we end a connection with force: false we have no way to force the end after the fact. So if we wanted to gracefully end something at first but then need to force it to end afterwards, we have no way of doing this.

On top of fixes we need to expand the features of a connection slightly here.

1. Allow the ability to force close all streams at any time to allow skipping the graceful end of streams when ending the connection with force: false
2. when ending a connection with force: false we don't want any new connections to be started. Any new connections from either direction should be rejected.

Issues Fixed

• No issues are directly fixed by this PR.
• Related: https://github.com/MatrixAI/Polykey/pull/609

Tasks

• [x] 1. When a connection is closed with force: false it should wait for all streams to gracefully end. Right now they throw an error.
• [x] 2. When a connection is ending, forced or otherwise, all new streams should be rejected. when force: false we should allow all active streams to end gracefully but no new ones should be created from either direction.
• [x] 3. When ending a connection with force: false we want the ability to force the end at any time. So we need a method of force closing all active streams at any time.

Final checklist

• [x] Domain specific tests
• [x] Full tests
• [x] Updated inline-comment documentation
• [x] Lint fixed
• [x] Squash and rebased
• [x] Sanity check the final build

[tegefaulkes]

Only task 1 needs to be addressed for https://github.com/MatrixAI/Polykey/pull/609. the rest are nice to have.

[ghost]

👇 Click on the image for a new way to code review

#### [](https://app.codesee.io/r/reviews?pr=75&src=https%3A%2F%2Fgithub.com%2FMatrixAI%2Fjs-quic) #### Legend

[CMCDragonkai]

Only task 1 needs to be addressed for MatrixAI/Polykey#609. the rest are nice to have.

This PR is for all 3 tasks right?

[tegefaulkes]

Yes, for now. we can extract any of the extra ones into a new issue if they're not strictly needed now.

[tegefaulkes]

Ok, when a connection is stopping then any call to streamNew throws the ready error. This was already a thing. But now any new incoming stream is shut down with a code of 1. this means that any remotely created stream will still be created, but any use of it will result in a error on that stream. There's no real way around this.

I don't like that it's a hard coded code of 1. Ideally the application can specify the code it wants for a rejected stream. Conversely, the code of 1 generally means generic or unknown error even on the application level. So it's kind of fine.

I guess part of the problem is that the behaviour is a little asymetric now. On the connection that is already stopping the streamNew will throw with the ready error. On the peer connection the stream itself will throw the error. I can't really prevent the peer from creating the stream in the first place, since from it's perspective the connection isn't ending yet.

Thoughts?

[CMCDragonkai]

It's fine to have a set of codes with some predetermined meaning.

We have transport codes and application codes.

Is this an application code? And then should the library default it? Should it be configurable? Make a judgement call here.

[tegefaulkes]

I've added a forceDestroyStreams method (name subject to change) to the QUICConnection, QUICClient and QUICServer. This allows you to cancel all active streams at any time. The main use for this is if we destroy any of these with force: false we can then later force it to finish destroying by destroying all the streams they're waiting for early.

[CMCDragonkai]

I've added a forceDestroyStreams method (name subject to change) to the QUICConnection, QUICClient and QUICServer. This allows you to cancel all active streams at any time. The main use for this is if we destroy any of these with force: false we can then later force it to finish destroying by destroying all the streams they're waiting for early.

Why does each 3 need this method? Only QUICConnection needs to manage streams. And if it is not being called elsewhere, why factor it out of the relevant destroy or stop methods?

[CMCDragonkai]

Do you want to squash some of the WIP commits?

[tegefaulkes]

I was going to squash this after any review changes were done.

Why does each 3 need this method? All 3 can be ended with force: false, in this case all 3 need a way to cancel all running streams if they want to upgrade from unforced end to a forced one.

[CMCDragonkai]

Is there any way to centralise that logic to QUICConnection? I don't think it should be necessary to manage streams in the other ones?

[tegefaulkes]

The QUICConnection is the one cancelling the streams directly. The new methods in the server and client just call forceDestroyStreams for every connection it's managing. The methods could be called something else on the server and client.

[CMCDragonkai]

This is just to destroy all streams while keeping the connection alive? No need to expose it. Why not keep it to the connection?

[tegefaulkes]

This is just to destroy all streams while keeping the connection alive? No need to expose it. Why not keep it to the connection?

It's mainly to allow you to force the server, client or connection to end now if it was waiting for streams to gracefully end during an unforced end. In reality it allows you to kill all active streams at any time.

It needs to be exposed on the client and server so we can force it to stop or destroy of we initially did an unforced stop or destroy.

Mainly needed by the server since you have direct access to the connection on the client but getting the connections on the server is a little more annoying.

[tegefaulkes]

After discussion.

1. forceDestroyStreams has been removed from the client and server.
2. forceDestroyStreams was renamed to destroyStreams.
3. It is only mean to be a debug method for the QUICConnection now. If you need to use it then you need to access the connection directly.
4. This is subject to further refactoring later.