Review - Tests, Key and Certificate Generation, Concurrency Control, Custom TLS Verification

Description
This is a review PR for the tests and crypto code that is being used in the tests.
Issues Fixed

Related #1

Tasks

[x] 1. Remove all the tests/fixtures in favour of auto-generated certificates and keys
[x] 2. Replace with the new key generation that is more flexible
[x] 3. Replace with certificate generation that supports RSA, ECDSA and Ed25519 keys
[x] 4. Review why Ed25519 certs have a slow shutdown - only specific cases where the client fails to verify the server TLS certificate and also for ECDSA, there is a 3 second timeout at the end, this is built into quiche.
[ ] 5. Review why there might be stream lifecycle problems when we increase the stream concurrency between connections. 2 days
[x] 8. Figure out if js-timer can be used for repetition (like reset the time and run again the handler). 0.5 days
[x] 6. Use js-timer for keepAliveIntervalTimer. 0.5 days
[x] 7. Use js-timer for connTimeoutTimer. 0.5 days
[x] 9. Custom TLS verification. 3days, 0.5-1 day left

Native tests - have the native callback set, and then run the JS callback directly
JS tests
Send the ack eliciting frame after verification or after sending a short frame
This sort of proves that both sides are fully verified for our connection establishment

[x] 10. Native tests required for stream life cycle, this will prove how the streams actually work internally. 1 day
[ ] 11. Test with respect to https://github.com/MatrixAI/js-quic/pull/26#issuecomment-1562382745 - test out the max idle timeout situation and also the high water mark of 0, ensure that no JS buffering of streams is occurring. 1 day
[x] 12. Monitor integration into the atomic set of operations discussed here https://github.com/MatrixAI/js-quic/pull/26#issuecomment-1576069312 - required parameter because QUICConnection methods there are all internal. 1 day
[x] 13. maxIdleTimeout needs to default to 1 min, the timeout for starting a connection and the keep alive needs to be strictly less than the maxIdleTimeout. If maxIdleTimeout is 0 then this constraint is ignored.
[ ] 14. The flow of events for the connection should be diagrammed with an activity diagram: https://github.com/MatrixAI/js-quic/pull/26#issuecomment-1576069312 - need to verify all these steps here

Final checklist

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

I've come to the conclusion that JsonWebKey is a far better key object to be used for portability even in-memory.
Right now in PK we are still using in-memory buffers, and that's fine, but if we want flexibility, then JsonWebKey is far superior.
So right now I've actually removing tests/tlsUtils.ts and replacing it with more generic functions in just tests/utils.ts that will be able to generate RSA, ECDSA, and Ed25519 keypairs and certificates while using JsonWebKey as the main portable format.
Note that it's easier to protect the memory of keys if they are buffers. But more difficult to do so for JsonWebKey objects. So in PK we only use Ed25519 keys so that's fine. But if we want to support more keypairs, then it may be a good idea to bring in the code I have right now and integrate it into PK.

The main libraries required here are just:

@peculiar/webcrypto
@peculiar/x509

That's it.
The ASN1 libraries are not needed. This is because we solely rely on peculiar's webcrypto to do everything.
The only reason PK is using the ASN1 libraries is to prepare to be more portable and eventually not rely on @peculiar/webcrypto when we are on mobile operating systems.

So in the fixtures, you generate regular keys and certs, and also you generate CAs.
I'm going to do this all dynamically now. Even CA signing is just a matter of having 2 keypairs.

@tegefaulkes I've noticed that you don't have any tests for QUICServer. No QUICServer.test.ts? Why is this?

I'm creating these.

@tegefaulkes make sure that exported types are at the bottom.

Most tests for QUICServer depend heavily on the QUICClient. they are tested as a pair in the QUICClient.test.ts test file. That one test file has gotten pretty large, We could split it up and rename it. But I'm not sure how many tests we can make that test the QUICClient or QUICServer in isolation.

The way you do tests between client and server is that server just runs tests on starting the server, with utility functions creating the client. In this sense you are "fixing the client", and testing variation on the server.
On the client, you "fix the server" and you test variations on the client.
Yes they can be tested together, but it depends on how you create tests. Whenever you're testing things you must pick your independent variables and dependent variables.
Ideally you fix everything except one thing. That reduces the amount of state space you need to cover.
Of course the more independent variables that exist, the more multidimensional variation that can exist, that's why we use fast check too to test higher dimensional spaces of configuration.
But you always start with the simplest smallest stuff first before building up to the more complex tests, because the first smaller tests are your sanity check.

@tegefaulkes types should always be in types.ts. You put TlsConfig into config.ts.

Here's an interesting idea for testing UDP sockets.
There's an option called reuseAddr.
Right now QUICSocket sets this to false... but why? It doesn't really matter to us if if something else were to bind to this port as well? Maybe there's a security issue here, but we could expose this as a configuration option.
Anyway...  this enables the ability for another socket to bind to the same address/port. Or it even enables running the QUICSocket on a port something else is using at the same time. Thus enabling 2 QUICSocket on the same port.
Subsequently, you can now send a message to the port, and I believe the message is duplicated to both sockets.
This allows you have 2 sockets that receive the same message sent to it.
This can enable some interesting tests, so that one can also get a copy of the messages that is being sent to the UDP socket used by QUICSocket.

I'm adding reuseAddr as an additional option to QUICSocket.start that defaults to false as normal.
But we can enable it for testing, or if there are special reasons to have this later in Polykey.

On QUICServer if the the socket is not shared, it will expose reuseAddr option too to be set. Again defaulting to false.

Ok the above idea means that with unicast packets, it only gets sent to the last bound socket. So I don't get both packets on both ports.
Basically by doing the above, the new socket will end up taking over, and messages get sent the new socket.
However IF I change QUICSocket to bind to <IP_ADDRESS> which binds to all interfaces, it will end up receiving the broadcast packet.
So the only way to get a fan-out structure is to have all sockets bound on <IP_ADDRESS> and then send a broadcast packet to <IP_ADDRESS>.
Now you end up getting packets on the shared sockets.

Anyway I'm not sure if we can use this unless we are sending broadcast packets. So I'll leave that solution to be explored later.
I tried also sending 2 unicast packets, both end up being sent to the same socket. I thought that there might be round-robin, but now I'm not sure.

One of the challenges using fast check with webcrypto is that we cannot seed webcrypto API.
In PK we are able to do this with Ed25519 because the private key is just a random bunch of bytes.
However for RSA, ECDSA, this is not possible.
So for now I don't think we will be using fast check on our keys and certificates, instead we will just always randomly generate them or use them across a number of tests.

Therefore, rather than generating the keys and certs all the time, we can just do it once at the beginning of each test module.
Something like this:
    let keyPairRSA: {
      publicKey: JsonWebKey;
      privateKey: JsonWebKey;
    };
    let certRSA: X509Certificate;
    let keyPairRSAPEM: {
      publicKey: string;
      privateKey: string;
    };
    let certRSAPEM: string;
    let keyPairECDSA: {
      publicKey: JsonWebKey;
      privateKey: JsonWebKey;
    };
    let certECDSA: X509Certificate;
    let keyPairECDSAPEM: {
      publicKey: string;
      privateKey: string;
    };
    let certECDSAPEM: string;
    let keyPairEd25519: {
      publicKey: JsonWebKey;
      privateKey: JsonWebKey;
    };
    let certEd25519: X509Certificate;
    let keyPairEd25519PEM: {
      publicKey: string;
      privateKey: string;
    };
    let certEd25519PEM: string;
    beforeAll(async () => {
      keyPairRSA = await testsUtils.generateKeyPairRSA();
      certRSA = await testsUtils.generateCertificate({
        certId: '0',
        subjectKeyPair: keyPairRSA,
        issuerPrivateKey: keyPairRSA.privateKey,
        duration: 60 * 60 * 24 * 365 * 10,
      });
      keyPairRSAPEM = await testsUtils.keyPairRSAtoPEM(keyPairRSA);
      certRSAPEM = testsUtils.certToPEM(certRSA);
      keyPairECDSA = await testsUtils.generateKeyPairECDSA();
      certECDSA = await testsUtils.generateCertificate({
        certId: '0',
        subjectKeyPair: keyPairECDSA,
        issuerPrivateKey: keyPairECDSA.privateKey,
        duration: 60 * 60 * 24 * 365 * 10,
      });
      keyPairECDSAPEM = await testsUtils.keyPairECDSAtoPEM(keyPairECDSA);
      certECDSAPEM = testsUtils.certToPEM(certECDSA);
      keyPairEd25519 = await testsUtils.generateKeyPairEd25519();
      certEd25519 = await testsUtils.generateCertificate({
        certId: '0',
        subjectKeyPair: keyPairEd25519,
        issuerPrivateKey: keyPairEd25519.privateKey,
        duration: 60 * 60 * 24 * 365 * 10,
      });
      keyPairEd25519PEM = await testsUtils.keyPairEd25519ToPEM(keyPairEd25519);
      certEd25519PEM = testsUtils.certToPEM(certEd25519);
    });

Once you can set multiple keys and certs, it's possible that it's the TLS client that figures out which key/cert to use to verify. This is something that's worth considering since we are going to enable Ed25519 here.

So boringssl does use the same environment variables:
»» ~/Projects/boringssl
 ♖ ag 'SSL_CERT_DIR'                                                                            (master) pts/7 18:37:51
crypto/x509/x509_def.c
71:#define X509_CERT_DIR_EVP "SSL_CERT_DIR"
»» ~/Projects/boringssl
 ♖ ag 'SSL_CERT_FILE'                                                                           (master) pts/7 18:37:56
crypto/x509/x509_def.c
72:#define X509_CERT_FILE_EVP "SSL_CERT_FILE"
»» ~/Projects/boringssl
 ♖ less crypto/x509/x509_def.c                                                                  (master) pts/7 18:38:01
»» ~/Projects/boringssl
 ♖ ag 'X509_CERT_DIR_EVP'                                                                       (master) pts/7 18:38:06
crypto/x509/x509_def.c
71:#define X509_CERT_DIR_EVP "SSL_CERT_DIR"
82:const char *X509_get_default_cert_dir_env(void) { return X509_CERT_DIR_EVP; }

That does mean that without setting the CA cert deliberately, it just relies on the default ca cert location on the operating system. On NixOS, that would be dependent on the cacert package.
This will be important when pushing the docker container image.

I refactored to something like this:
    // Setup all certificates and keys
    // The below may not actually work
    // We assume we can just use certificate and add them to it
    // However this may not be possible
    if let (Some(key), Some(cert)) = (key, cert) {
      for (k, c) in key.iter().zip(cert.iter()) {
        let private_key = boring::pkey::PKey::private_key_from_pem(&k)
          .or_else(
          |err| Err(Error::from_reason(err.to_string()))
        )?;
        ssl_ctx_builder.set_private_key(&private_key).or_else(
          |e| Err(napi::Error::from_reason(e.to_string()))
        )?;
        let x509_cert_chain = boring::x509::X509::stack_from_pem(
          &c.to_vec()
        ).or_else(
          |err| Err(napi::Error::from_reason(err.to_string()))
        )?;
        for (i, cert) in x509_cert_chain.iter().enumerate() {
          if i == 0 {
            ssl_ctx_builder.set_certificate(cert,).or_else(
              |err| Err(Error::from_reason(err.to_string()))
            )?;
          } else {
            ssl_ctx_builder.add_extra_chain_cert(
              cert.clone(),
            ).or_else(
              |err| Err(Error::from_reason(err.to_string()))
            )?;
          }
        }
      }
    }

Yea I'm pretty sure that the current boring crate does not provide a straight forward way to do this multiple certificate chain.
Also I did find a good resource that explains what all the boringssl functions do: https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html. It's much more clearer than all the other manpages.
In particular this https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_CTX_set_cert_cb is actually one of the main ways in which one could potentially select the correct certificate.
So for now I'm going to disable the ability to set multiple certificates on the Rust side.

Ok config tests are done for now. No more TlsConfig anymore. It's just ca, key and cert.

I've started documenting the config more clearly. Some of the parameters of quiche requires a deeper test and review on how they work.
In particular:

initialMaxStreamsBidi - we need to find out if this applies across the whole QUIC socket or on per-connection basis. Also what happens if you exceed it and correctly handle for this.
Setting the highWaterMark to 0, we don't want to do repeated buffering in this library. It's enough for quiche itself to already be doing buffering on incoming stream data.
Setting the default of maxIdleTimeout to 0, as per the default in quiche. This enables infinite timeout. Which means it's up to the user to decide if they want a smaller timeout. We can make use of this to enable the usage of js-async-cancellable to the explicitly cancel the connection creation. However we have to know how to actually cancel the connection creation. This would involve a change to the await Promise.race([connection.establishedP, errorP]), as it is currently intended to wait for a timeout error, if instead we use a 0 timeout, then the timeout is driven by JS instead of inside quiche.

I think the whole timeout looping is way too complicated as it is written. It requires some refactoring similar to what happened during the tasks domain in PK.

I drilled it down to this call:
    if (this.conn.isClosed()) {

Which is called at the beginning of QUICConnection.send.
This check takes 3000ms.
This function call goes directly the rust code. It's a synchronous function call which itself simply wraps the internal quiche::Connection::is_closed.
  #[napi]
  pub fn is_closed(&self) -> bool {
    return self.0.is_closed();
  }

The source code for quiche is:
    /// Returns true if the connection is closed.
    ///
    /// If this returns true, the connection object can be dropped.
    #[inline]
    pub fn is_closed(&self) -> bool {
        self.closed
    }

So it's just returning a boolean.
This is very strange, why would this call itself take 3 seconds to complete?

Interestingly even with RSA, checking that sole boolean takes 200ms..
I'm beginning to think this cannot be due to an FFI call.
There must be something happening that blocks the node thread..., perhaps there is some processing occurring that prevents the call from going through. CPU or IO contention...

I think something is wrong with the QUICConnection.checkTimeout function. The I started differentiating between client connection and server connnection, and I don't think it has anything to do with checking isClosed().
The checkTimeout function is extremely complex and I'm pretty sure there is something wrong here.

It turns out it's actually the time set for the setTimeout call. When it's using RSA keypair, it's 150ms. When it's ECDSA keypair, it becomes 2037ms.
Something is causing a longer time to be returned by this.conn.timeout().

[nix-shell:~/Projects/js-quic]$ npm run ts-node -- ./test-bootstrap.ts

><EMAIL_ADDRESS>ts-node
> ts-node ./test-bootstrap.ts

ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (0): null Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (1): 998 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (2): 998 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (3): 998 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (0): null Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (1): null Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (2): 998 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (4): 5 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (5): 17 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (6): 16 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (3): 980 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (4): 980 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (7): null Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (5): 2996 Thu May 25 2023 22:59:55 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (6): 2033 Thu May 25 2023 22:59:56 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 2de0517a-ab84a50b:THE TIME (7): null Thu May 25 2023 22:59:58 GMT+1000 (Australian Eastern Standard Time)
STOP SERVER: 3.001s

[nix-shell:~/Projects/js-quic]$ npm run ts-node -- ./test-bootstrap.ts

><EMAIL_ADDRESS>ts-node
> ts-node ./test-bootstrap.ts

ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (0): null Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (1): 998 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (2): 998 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (3): 998 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (0): null Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (1): null Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (2): 998 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (4): 8 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (5): 9 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (6): 8 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (7): 27 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (8): 26 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (3): 30 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (4): 30 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (5): 30 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (6): 30 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (9): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (10): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (11): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (12): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (13): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (14): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (15): 0 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICConnection--CLIENT CONNECTION--:THE TIME (16): null Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (7): 152 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (8): 150 Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
ERROR:QUICServer Test.QUICServer.QUICConnection 544f4176-b24bf603:THE TIME (9): null Thu May 25 2023 23:00:27 GMT+1000 (Australian Eastern Standard Time)
STOP SERVER: 156.417ms

I logged out both the RSA at the bottom and ECDSA at the top.
We can see that the time being returned becomes quite different from the 5th call onwards.

It turns out it's actually the time set for the setTimeout call. When it's using RSA keypair, it's 150ms. When it's ECDSA keypair, it becomes 2037ms.
Something is causing a longer time to be returned by this.conn.timeout().

I've seen this behaviour before. There is an existing issue for it. #11 It may be useful for reference.

One of the key behaviours that should be observed is that the timeout should return null only when either the connection is not yet opened, or if it has been closed.

From this point, we can see it's most likely an upstream issue with non-RSA certs. Their timeout values are just different when it's non-RSA.
However to solve this problem, I think we can do something different.
Our QUICConnection.destroy ultimately depends on await this.closedP.
Right now if you look at the logic of destroy, the steps it follows is:

Await concurrently destroy all streams in the stream map
Tell quiche that we are now closing the connection with this.conn.close
Await this.send()
Await this.closedP()

Waiting for this.closeP() depends on this.resolveCloseP() to being called.
The problem here is that originally we expected that upon calling await this.send() that would realise that the connection is closed after sending the last close frame. Therefore, we would expect that this.resolveCloseP to be called and thus allow await this.closeP to pass through.
I suspect that we should be able to change this logic to either change how QUICConnection.send works or by checking if the connection is closed in just the QUICConnection.destroy call.
It's just weird that we are waiting for the timeout to finish before we consider the connection to be closed.
It's also possible that we are actually waiting for the other side to close too.

So basically the problem is that we are waiting out the timeout for the connection to be destroyed.
However I don't think this is the source of the problem.
I've noticed that if I destroy the client connection ahead of time, then wait for 10000 ms, it still ends up taking the 3 seconds to shutdown the quic server.
This implies that the server side QUICConnection is not being closed when the client closes the QUICConnection.
This is a resource leak and indicates a protocol problem with how we are using the library.

To debug this... we need to create 2 separate QUICConnectionClient and QUICConnectionServer. Then create different functionality for both and observe the difference in the protocol.

The applicationError should default to true, since a graceful exit is actually an application CONNECTION_CLOSE.

I've proven that calling timeout during the constructor is useless. It's always null. There is no timeout until we trigger the first send or recv.
So I'm removing that call to the timeout in the constructor and adding a test for this.

The new tests tests/native/quiche.test.ts showcase all the conditional checks that occur during connect, accept and close.
We can see that timeout is always null at that point. Meaning that won't happen until there is a send or recv.
Also interestingly if you close the connection, because there is nothing "draining", then it immediately closes and so isClosed() returns true immediately.
I suspect that if we were able to craft the actual packets that QUIC expects, we could just directly do the read/write and more easily step-through the state changes.

If we were to lift the sendBuffer out to the top of the QUICConnection, we would be able to re-use the sendBuffer. However this can cause problems with concurrency with different calls clobbering the shared buffer.
Alternatively to this is to always use Buffer.alloc. That would actually be quicker. It would make sense to distribute this library with Buffer and not use Uint8Array here.

Ok already discovered another bug.
During the QUICSocket.handleSocketMessage we need to also handle BufferTooShort exceptions coming out of quiche.Header.fromSlice.
In both cases of BufferTooShort or InvalidPacket, these are considered peer errors, and we should just simply drop the handling there.
This is important for any hole punching but also in case random packets is arriving at the socket.

A possible concurrency bug exists in the QUICSocket.handleSocketMessage.
At the very end we effectively do this:

`await conn.recv(data, remoteInfo_);
await conn.send().

The problem here is that according to quiche, upon a recv call, the send must be called.
Because our code is asynchronous, it's possible that in between another recv call is made for the same connection. Whether this is actually a problem or not is a bit unknown. In the state machine testing of the connection lifecycle I can see different behaviour occur if I were to call recv twice or something.
To prevent this... we should lock the connection between await conn.recv and await conn.send. This ensures that it occurs together atomically one after another before another pair of recv and send is called.
Another thing is that I don't get ignoring the send errors. These function calls as they occur inside an event handler should never throw any exception anyway. So they this:
      // Ignore any errors, concurrent with destruction
      await conn.send().catch(() => {});

Is incorrect.

I'm going to add some dependencies:

@matrixai/async-cancellable - this will give us PromiseCancellable
@matrixai/async-locks - this will give us the Lock needed for locking on recv and send.
@matrixai/timer - this will give us Timer

We combine the timer and the PromiseCancellable to essentially decorate the async creation functions with the ctx pattern. Right now the decorator and HOF functions in PK but it hasn't been extracted out into its own library due usage of a shared contexts map. It was imagined that if contexts library is shared between libraries... that could be an issue. Anyway given that this library will be one the first to use (perhaps with MDNS @amydevs) then we can see how to extract that later.

Just trying to remember how this locking stuff works. We have DBTransaction which allows one to inherit the transaction and lock things in it as well. Here we don't have a DB transaction, so instead we have just a regular lock. This is usually used for some sort of synchronisation inside the class. but in other cases, we have use with....
Ok I think I remember. All the locking that was required to deal with databases was done with the DBTransaction. But with regular locks we just expose the Lock or LockBox.
Higher level methods are usually created to do things like withVault.
So here in the cause of QUICConnection, we can create a sendRecvLock: Lock = new Lock().
This however has to be used by QUICSocket, and so the classes are all sort of reaching into each other, the classes are tightly coupled.

If QUICConnection.acceptQUICConnection and QUICConnection.connectQUICConnection were to get an extra parameter of ctx: ContextTimed this has some weird behaviour.
On the Server Side
From the server side, the QUICConnection.acceptQUICConnection is called automatically being event driven from QUICSocket.handleSocketMessage and the QUICServer.connectionNew.
This means the user does not have any ability to pass a ctx in that case.
Any usage of the ctx would happen from QUICServer internally.
A ctx: ContextTimed would mean ctx: { signal, timer }.
We can see here that timer would not make much sense here because this would conflict with the usage of maxIdleTimeout configuration. Now if it is defaulted to 0, this means that the connection will never timeout on idle. However this time is also used when a connection is first created and the timeout started. If one would set the maxIdleTimeout to 5000 and the timer to new Timer(undefined, 6000), this would have no effect.
Now we cannot simply ignore the usage of maxIdleTimeout because to the timeout mechanism inside quiche which is more of a "tick" system for the purpose of doing state transition over clock time.
Therefore it must always end up being used. This means... for the purposes giving time-limiting connection establishment, the maxIdleTimeout must be used. Any usage of timer would only make sense in these situations:

If the timer is less than the max idle timeout, then we would want to explicitly close the connedtion before the max idle timeout.
It only makes sense to be used for the QUICConnection.connectQUICConnection because why would you preconfigure the server a timer that is less than the maxIdleTimeout?

Therefore it seems to mean that QUICConnection.acceptQUICConnection would only make use of ContextCancellable which is { signal: AbortSignal }.
Now again on the server side, the user is not driving this action. Therefore the signal would be automatically passed in from the QUICServer, and it would only be used to cancel the establishment of accepting connections. Which would only be used upon calling QUICServer.destroy({ force: true }).
In that scenario we are force closing everything, including streams and connections without any graceful exit.
On the Client Side
On the client side ContextTimed can be used. In fact this would be passed in directly from the QUICClient.createQUICClient function.
Waiting for Establishment
Now if the ctx is used here on both QUICConnection.connectQUICConnection and QUICConnection.acceptQUICConnection, then that means interally both of these async creation functions will need to wait for the establishedP promise to be resolved.
An establishment event only occurs after several back and forths.
For the client side it has to pass through these steps:

QUICConnection.connectQUICConnection
Send initial
Receive retry with token from server
Send initial with token
Receive initial from server
Send initial
Receive handshake from server
Now it is established
Send handshake

For the server side, it's something similar.
The problem here is that events 3, 5, 7 all happen from QUICSocket.handleSocketMessage. Waiting for the establishment event would mean that accepting connection is then blocking the original handleSocketMessage.
We do not want to keep the original QUICSocket.handleSocketMessage waiting for the connection to be established. It doesn't seem to fit in the scope of QUICSocket to be waiting.
For the client, it doesn't block the QUICSocket.handleSocketMessage, but again these events 3, 5, 7 would need come from QUICSocket.handleSocketMessage.
This is why currently we have socket.connectionMap.set(connection.connectionId, connection) inside both QUIConnection.acceptQUICConnection and QUICConnection.connectQUICConnection.
For events 3, 5, and 7 to work, the object instance of QUICConnection must already exist. If we are blocking on establishment, the QUICSocket must have access to the QUICConnection object before the return of the async creation functions.
This goes against our usual way of doing things where we would not expect the object instance to exist outside of the async creation function scope until it has returned.
It is possible to do this though because we do create the object instance, we set it to the socket.connectionMap and thus the QUICSocket would trigger recv and send in order transition the connection instance to established state.
However it's just a bit messy.
What if the ctx was on the QUICClient and QUICServer instead?
It's possible that instead of pushing the ctx all the way into QUICConnection, we instead leave it at the QUICClient.createQUICClient and QUICServer.connectionNew functions.
Now the async creation functions create the connection object, but the connection object won't be ready until it is transitioned to established state. The job of doing this will be in the QUICClient.createQUICClient and QUICServer.connectionNew.
This is actually closer to what we already have.
I think this should be attempted first.

@tegefaulkes I've noticed across the PK codebases there's quite a few places where ctx: ContextTimed isn't optional. Generally this should bean optional parameter.

So anyway this means the signature looks like this:
public static createQUICClient(opts, ctx?: Partial<ContextTimed>): PromiseCancellable<QUICClient>

I've omitted the opts type cause that's quite large. The ctx is therefore the last optional parameter. Ideally we could use our decorators... but right now cannot do that.
I might leave this functionality empty for now, because it's likely that I extract the decorators so that I can use it in js-quic. I realised that the decorators do in fact work for static methods.

To not block the QUICSocket.handleSocketMessage, we need to make sure that connectionNew isn't exactly being awaited for. Instead it would have to be function that is called synchronously. Effectively doing something like void quicServer.connectionNew() even if the function itself is asynchronous.
We would expect the function to not throw exceptions. It would represent a "state transition" on the QUICServer itself, which manages the existence of this connection. Internally it can await for the establishment event.

Actually the usage of signal is only needed if we are cancelling something asynchronous. There's nothing asynchronous tracking the establishedP on the server side. Is this actually a problem? I'm not entirely sure because if the max idle timeout is 0, then you can infinitely wait. If it's not, then the idle timeout will eventually destroy the connection. Furthermore if the server is being destroyed, then it force destroy connections too. There's nothing asynchronous that needs to be cancelled.
In that case, the only usage of ctx is on QUICClient.createQUICClient and therefore nowhere else.

I've extracted out the contexts domain from PK and put it into https://github.com/MatrixAI/js-contexts.
So far it's pretty good.
@amydevs please watch that repo to understand what needs to be updated relative to TypeScript-Demo-Lib. I noticed that some things are outdated in TS demo lib.
Now I've noticed a problem. Upgrading to node v18.15 from v16.15 results in some test failures of the contexts.

This only happens when changing to node v18.15.
I'll need to check why this is the case and update the contexts library since it will be used by js-quic which is using v18.15 and will be propagated to Polykey too.

I can see the source of the error. With the new node v18.15. When rejecting the promise cancellable we now get a DOMException.
    PromiseCancellable {
      <rejected> DOMException [AbortError]: This operation was aborted
          at new DOMException (node:internal/per_context/domexception:53:5)
          at AbortController.abort (node:internal/abort_controller:336:18)
          at PromiseCancellable.cancel (/home/cmcdragonkai/Projects/js-contexts/node_modules/@matrixai/async-cancellable/src/PromiseCancellable.ts:205:26)
          at Object.<anonymous> (/home/cmcdragonkai/Projects/js-contexts/tests/functions/cancellable.test.ts:41:10),
      reject: [Function (anonymous)],
      abortController: AbortController { signal: AbortSignal { aborted: true } }
    }

I remember this error. Basically by default, the abort is supposed to abort with an AbortException as per https://developer.mozilla.org/en-US/docs/Web/API/AbortController/abort.
However this behaviour was probably not correct in Node.js on v16.15.
@tegefaulkes we've seen this problem before. But I forgot how we solved that.
What's weird is that we actually updated to v18 in Polykey already and the contexts tests work there.

Actually v16 was just wrong. The right answer is that it uses the generic AbortError.

Ok the fix to this is just to go js-async-cancellable and update it. What we can do is detect when an undefined is passed to cancel. Then pass in a internal symbol as the reason, that we can then change back to undefined when we go to reject the promise.

This was meant to be fixed for 1.0.4 @tegefaulkes but it doesn't appear to be fixed.

This was not fixed in 1.0.4. The fix there only explicitly cancels with a symbol now.
In fact 1.0.4 is basically incorrect, it does not fix the problem?? @tegefaulkes

I'm releasing 1.0.5 so that the problem is actually fixed using the symbol trick.

I'm pushing up 1.0.6. The fix for it is a bit tricky. Swapping for undefined only works on the rejection handler. However intermediate signals will end up seeing the abortUndefinedReason as the reason. It's only when it is finally rejected does it swap to undefined.
We could also change our expectations to say that it will just now reject with DOMException, but I don't like this as this is breaking change and it is not what is expected when cancelling a PromiseCancellable.

We now have @matrixai/contexts at v1.0.0.

I've discovered that there could be a clock delay between the JS and Rust code. This can cause timing bugs.
Basically if Rust tells us to wait 118ms, if we use this with a sleep of 118ms and then call conn.onTimeout(), we might expect that something should happen.
However it's possible that upon calling onTimeout(), from Rust's perspective not enough time has passed yet.
Therefore the only foolproof way of ensuring that Rust really thinks the time has passed is to call conn.timeout() again until it has reached 0.
Only at that point, can we be sure that Rust's thinks the time has passed enough. We may only be off by 1ms. And then we can do the conditional checks like conn.isClosed() or conn.isDraining() and ensure deterministic behaviour.
I found this while testing the timeout in the case of connection close, and then checking whether the connection is closed. If I added 1 to the time I'm waiting for, it would work, but this would not be reliable compared to just spin-looping the timeout call until it reaches 0.
One question though, what if the timeout call returns null? Generally this should not happen. If there was a timeout, and we keep calling it, it will only go down to 0. The state has not transitioned yet, so it should not give us null, not until we call conn.onTimeout().

A utility like this should suffice:
async function waitForTimeout(conn): Promise<void> {
  while (true) {
    const timeout = conn.timeout();
    if (timeout === 0 || timeout == null) {
      return;
    }
    await sleep(timeout);
  }
}

Actually even that utility function isn't enough.
I've tried sleeping for 0 seconds, yield micro with queueMicrotask and also just waiting it reach 0. It's simply possible for the timeout to not be exhausted.
Interestingly, if the timeout() where to return a number we would end up looping back anyway. So maybe we don't need to wait for 0, instead... we actually need to wait for null.

I've now tested a full connection lifecycle for both RSA, ECDSA and Ed25519 using step-by-step transitions.
Here is the reults:
 PASS  tests/native/quiche.test.ts
  quiche
    ✓ frame parsing (37 ms)
    connection lifecycle
      connect and close client connection
        ✓ connect (3 ms)
        ✓ close (2 ms)
      establish connection between client and server with RSA
        ✓ connect (4 ms)
        ✓ dialing (1050 ms)
        ✓ negotiation (10 ms)
        ✓ established (32 ms)
        ✓ close (289 ms)
      establish connection between client and server with ECDSA
        ✓ connect (1 ms)
        ✓ dialing (1051 ms)
        ✓ negotiation (9 ms)
        ✓ established (14 ms)
        ✓ close (137 ms)
      establish connection between client and server with Ed25519
        ✓ connect (1 ms)
        ✓ dialing (1051 ms)
        ✓ negotiation (7 ms)
        ✓ established (14 ms)
        ✓ close (139 ms)

As you can see... there's actually no difference in the timeouts. They are very close. In fact in this case, it takes longer to close off the RSA.
I'm going to split up the established part of the test a bit more into chunks that can be re-used.
Interestingly, only RSA requires the handshake frames. The other 2 ECDSA and Ed25519 do not require additional handshake frames. It's not necessary, they are established quicker... and in this case, close quicker too.

Even though this seems like model based test to use fast check with, the problem is that we don't really fully understand the model of QUIC, plus I need precise control here to debug this.

They are now separated, and we can see what is the diff between RSA and the others:
 PASS  tests/native/quiche.test.ts
  quiche
    ✓ frame parsing (31 ms)
    connection lifecycle
      connect and close client connection
        ✓ connect (3 ms)
        ✓ close (5 ms)
      connection between client and server with RSA
        ✓ client connect (2 ms)
        ✓ client dialing (1050 ms)
        ✓ client and server negotiation (9 ms)
        ✓ server accept (5 ms)
        ✓ client <-initial- server (6 ms)
        ✓ client -initial-> server (2 ms)
        ✓ client <-handshake- server (4 ms)
        ✓ client is established
        ✓ client -handshake-> server (3 ms)
        ✓ server is established
        ✓ client <-short- server (2 ms)
        ✓ client -short-> server (3 ms)
        ✓ client and server established (1 ms)
        ✓ client close (199 ms)
      connection between client and server with ECDSA
        ✓ client connect (1 ms)
        ✓ client dialing (1052 ms)
        ✓ client and server negotiation (7 ms)
        ✓ server accept (3 ms)
        ✓ client <-initial- server (3 ms)
        ✓ client is established
        ✓ client -initial-> server (1 ms)
        ✓ server is established
        ✓ client <-short- server
        ✓ client -short-> server (3 ms)
        ✓ client and server established
        ✓ client close (137 ms)
      connection between client and server with Ed25519
        ✓ client connect (1 ms)
        ✓ client dialing (1051 ms)
        ✓ client and server negotiation (6 ms)
        ✓ server accept (3 ms)
        ✓ client <-initial- server (3 ms)
        ✓ client is established
        ✓ client -initial-> server (1 ms)
        ✓ server is established
        ✓ client <-short- server (1 ms)
        ✓ client -short-> server (2 ms)
        ✓ client and server established
        ✓ client close (130 ms)

We now have step through tests for timeouts that occur during dialing, initial, handshake and established.

I think a few more tests that test the verification behaviour of TLS certificates is required... then we can proceed with updating the underlying logic of the QUIC code would be in order.
@tegefaulkes if you're finished looking into the design of integration and you've written the spec on how integration needs to be done, we can discuss how to divvy up the work here in the QUIC system to address all the issues that was discovered.

Ok we have all the tests I think we need regarding lifecycle and TLS behaviour. I can now replicate the exact situation where there is a longer delay in closing connections in ECDSA and Ed25519.
 PASS  tests/native/quiche.test.ts
  quiche
    ✓ frame parsing (3 ms)

 PASS  tests/native/quiche.tls.test.ts (9.084 s)
  quiche tls
    RSA success
      ✓ client connect (1 ms)
      ✓ client dialing (1 ms)
      ✓ client and server negotiation (4 ms)
      ✓ server accept (2 ms)
      ✓ client <-initial- server
      ✓ client -initial-> server (1 ms)
      ✓ client <-handshake- server (2 ms)
      ✓ client is established (1 ms)
      ✓ client -handshake-> server
      ✓ server is established (1 ms)
      ✓ client <-short- server
      ✓ client -short-> server
      ✓ client and server established (1 ms)
      ✓ client close (59 ms)
    RSA fail verifying client
      ✓ client connect
      ✓ client dialing (1 ms)
      ✓ client and server negotiation (1 ms)
      ✓ server accept (2 ms)
      ✓ client <-initial- server (1 ms)
      ✓ client -initial-> server
      ✓ client <-handshake- server (2 ms)
      ✓ client is established
      ✓ client -handshake-> server (10 ms)
      ✓ client <-handshake- server (2 ms)
      ✓ client and server close (28 ms)
    RSA fail verifying server
      ✓ client connect
      ✓ client dialing (1 ms)
      ✓ client and server negotiation (1 ms)
      ✓ server accept (2 ms)
      ✓ client <-initial- server
      ✓ client -initial-> server (1 ms)
      ✓ client <-handshake- server (1 ms)
      ✓ client -handshake-> server (2 ms)
      ✓ client and server close (26 ms)
    ECDSA success
      ✓ client connect
      ✓ client dialing
      ✓ client and server negotiation (1 ms)
      ✓ server accept
      ✓ client <-initial- server (1 ms)
      ✓ client is established
      ✓ client -initial-> server (1 ms)
      ✓ server is established
      ✓ client <-short- server
      ✓ client -short-> server (1 ms)
      ✓ client and server established
      ✓ client close (23 ms)
    ECDSA fail verifying client
      ✓ client connect
      ✓ client dialing
      ✓ client and server negotiation (2 ms)
      ✓ server accept
      ✓ client <-initial- server
      ✓ client is established
      ✓ client -initial-> server (2 ms)
      ✓ client <-handshake- server (2 ms)
      ✓ client and server close (14 ms)
    ECDSA fail verifying server
      ✓ client connect
      ✓ client dialing
      ✓ client and server negotiation (2 ms)
      ✓ server accept
      ✓ client <-initial- server (2 ms)
      ✓ client -initial-> server (3 ms)
      ✓ client and server close (2995 ms)
    Ed25519 success
      ✓ client connect
      ✓ client dialing
      ✓ client and server negotiation (1 ms)
      ✓ server accept (1 ms)
      ✓ client <-initial- server
      ✓ client is established (1 ms)
      ✓ client -initial-> server
      ✓ server is established
      ✓ client <-short- server (1 ms)
      ✓ client -short-> server
      ✓ client and server established (1 ms)
      ✓ client close (23 ms)
    Ed25519 fail verifying client
      ✓ client connect (1 ms)
      ✓ client dialing
      ✓ client and server negotiation (1 ms)
      ✓ server accept (1 ms)
      ✓ client <-initial- server
      ✓ client is established
      ✓ client -initial-> server (2 ms)
      ✓ client <-handshake- server (2 ms)
      ✓ client and server close (13 ms)
    Ed25519 fail verifying server
      ✓ client connect
      ✓ client dialing
      ✓ client and server negotiation (2 ms)
      ✓ server accept
      ✓ client <-initial- server (2 ms)
      ✓ client -initial-> server (2 ms)
      ✓ client and server close (2997 ms)

 PASS  tests/native/quiche.connection.lifecycle.test.ts (16.507 s)
  quiche connection lifecycle
    connection lifecycle
      connect and close client
        ✓ connect (5 ms)
        ✓ close (5 ms)
      connection timeouts
        dialing timeout
          ✓ client connect (1 ms)
          ✓ client dialing timeout (2998 ms)
        initial timeout
          ✓ client connect
          ✓ client dialing (1 ms)
          ✓ client and server negotiation (3 ms)
          ✓ server accept (2 ms)
          ✓ client <-initial- server timeout (3000 ms)
        handshake timeout
          ✓ client connect
          ✓ client dialing
          ✓ client and server negotiation (2 ms)
          ✓ server accept (2 ms)
          ✓ client <-initial- server
          ✓ client -initial-> server (1 ms)
          ✓ client <-handshake- server timeout (2003 ms)
        established timeout
          ✓ client connect (1 ms)
          ✓ client dialing
          ✓ client and server negotiation (1 ms)
          ✓ server accept (2 ms)
          ✓ client <-initial- server (1 ms)
          ✓ client -initial-> server
          ✓ client <-handshake- server
          ✓ client is established
          ✓ client -handshake-> sever
          ✓ server is established
          ✓ client <-short- server timeout (2011 ms)
      connection between client and server with RSA
        ✓ client connect (8 ms)
        ✓ client dialing (1052 ms)
        ✓ client and server negotiation (11 ms)
        ✓ server accept (4 ms)
        ✓ client <-initial- server (3 ms)
        ✓ client -initial-> server (2 ms)
        ✓ client <-handshake- server (5 ms)
        ✓ client is established (1 ms)
        ✓ client -handshake-> server (3 ms)
        ✓ server is established
        ✓ client <-short- server (2 ms)
        ✓ client -short-> server (4 ms)
        ✓ client and server established (1 ms)
        ✓ client close (165 ms)
      connection between client and server with ECDSA
        ✓ client connect (2 ms)
        ✓ client dialing (1051 ms)
        ✓ client and server negotiation (8 ms)
        ✓ server accept (3 ms)
        ✓ client <-initial- server (3 ms)
        ✓ client is established
        ✓ client -initial-> server (1 ms)
        ✓ server is established
        ✓ client <-short- server (1 ms)
        ✓ client -short-> server (3 ms)
        ✓ client and server established (1 ms)
        ✓ client close (136 ms)
      connection between client and server with Ed25519
        ✓ client connect (1 ms)
        ✓ client dialing (1051 ms)
        ✓ client and server negotiation (7 ms)
        ✓ server accept (4 ms)
        ✓ client <-initial- server (3 ms)
        ✓ client is established
        ✓ client -initial-> server (2 ms)
        ✓ server is established
        ✓ client <-short- server (1 ms)
        ✓ client -short-> server (3 ms)
        ✓ client and server established (1 ms)
        ✓ client close (130 ms)

Test Suites: 3 passed, 3 total
Tests:       156 passed, 156 total
Snapshots:   0 total
Time:        17.03 s

I can see that actually the only situation when the longer delay happens is due to to MTLS failing the server. And thus resulting in a longer timeout on the server side to "drain" their connections. Outside of this, closing connections is pretty similar. This is pretty cool, because now we have a straightforward reproduction of the problem that can be reported upstream.

The problem is very similar to an upstream issue I've already posted. https://github.com/cloudflare/quiche/issues/1488

Ok so there are some tasks we need to do here...

We need to only call the setTimeout upon recv or send. ACTUALLY since we always either are calling send after a recv, or just shutting down, we could just only call it on send.
We need the recv and send combo to be atomic, and mutually exclusive with regards to other state transitions that could happen to the connection.
We need to change our setTimeout to always loop even if the conn.timeout() gives us 0. It has to basically do a set timeout loop until it results in a null. Even if it gives us a 0we have to do sleep for that and then callonTimeout()`.
Any call to setTimeout updates the the timer with the new conn.timeout() always. We do not compare the longer time or the shorter time.
The recv and send combo is atomic, which means we cannot have us calling the onTimeout in between. This means there needs to be a state transition lock. This lock is locked whenever we are in the midst of a state transition. So that even set timeout functions are queued up.
The destroy function needs to be redone so that it's clear what it is doing into separate blocks/chunks that is switched with explicit condition checks. It's important that when a close frame is sent, that must result in a send call being made as well. This would need to also lock the state transition lock.
Event bubbling for all the objects should be approached like the DOM.

I'm changing the handshakeP to secureEstablishedP. This makes it similar to connection and secureConnection event in the tls module in Node.js.
Now the secureEstablishedP is a bit tricky. This happens after establishedP, but when exactly?
Well depending on RSA, ECDSA or Ed25519 it's all a bit different.
Technically secureEstablishedP on the client should only happen if the client is established AND we the client knows that the connection is also established on the server.
And for the server, it should only happen if the server is established and we the server knows that the connection is also established on the client.
The first condition for both is easy. But the second condition depends on the packet type being sent, and not resulting in a TlsFail.
One pattern above is waiting for the first short frame to arrive. But I'm not confident that this is always the case, plus it's a bit weird using this as the indication of secureEstablishedP since short frames are just generic frames.
However we do know that short frames can only occur after the connection is established.
Perhaps we can do something like, upon the first short frame successfully received AFTER established, then we can resolve this...
Actually looking at this again. It appears that on the server side, being established means you are secure established.
But on the client, being established doesn't mean you're secure established. You have to successfully process the next frame, assuming the next frame is a short frame.

I just want to point out that as well as createQUICClient being cancellable. The server initHolePunch needs to be cancellable as well. But @CMCDragonkai pointed out that initHoldPunch should be relatively simple and the reverse dialing logic should be implemented in Polykey instead.

Regarding the event bubbling idea.
I noticed that with EventTarget, you can have an event type being the name and the event that you are emitting.
This creates a few problems.
For example I may want to have a QUICStream destroy event.
If I were to bubble this event up, and the type was destroy, this type would end up having the same name as the destroy event on the QUICConnection.
So if you do quicConn.addEventListener('destroy') you would end up getting both events.
Now you could differentiate which destroy you are getting by subsequently checking either the event target, or in our case we can check the actual instanceof of the event to know what we are talking about.
However this departs quite alot from regular event listeners. If I add an event listener for destroy to the QUICServer, I would not expect that the destruction of the stream would trigger that and I would have to check the event internals to know it.
This is technically different from the DOM model where a click event bubbles up the DOM and you have to check the event target to know which target you're talking about. The DOM is a bit different because these events sort of makes sense to overlap. A click on the button is still a click on the entire div box containing the button.
But the intuition doesn't make sense for our system.

I've pruned the events to be precisely these:
  QUICSocketStopEvent,
  QUICSocketErrorEvent,
  QUICClientDestroyEvent,
  QUICClientErrorEvent,
  QUICServerConnectionEvent,
  QUICServerStopEvent,
  QUICServerErrorEvent,
  QUICConnectionStreamEvent,
  QUICConnectionDestroyEvent,
  QUICConnectionErrorEvent,
  QUICStreamDestroyEvent,

I removed the readable, writable, recv, and send events because they are not necessary. They are just regular function calls. Events bubble up out to the user, we don't use events internally for coordination inside js-quic.

In the sense of bubbling. All those events above bubble to QUICClient and QUICServer.
One important thing is the QUICSocket events, because it can be encapsulated or not... we have to make a decision as to whether to re-emit those events to the QUICClient or QUICServer.
Atm, they only emit if it is encapsulated. However... I've been thinking that to make it simpler, it should always re-emit to the client or server, no matter how many clients or optionally a server are using the same socket.

The event types change a bit too. The e.detail now always refers to an Error type. We always emit the original event first, then remit as its own error. Because the client wraps both the socket and connection.
The server however wraps the socket but does not wrap the connection. So server error wraps socket error, but does not wrap connection errors.

I have a feeling it's possible to change QUICConnection to instead by StartStop pattern.
The reason is because you need to be able to construct it to be able to use the conn.recv and conn.send functions.
If you had a CreateDestroyStartStop, the Create would need to call start. And we don't want to do that.
Anyway that means await conn.start() means you are awaiting for the secureEstablishedP. And await conn.stop() means you are awaiting for closedP.
Then the @ready can be used for functions that only make sense when the connection is securely established.

I need to remove the crypto requirement of QUICClient because it's only needed by the server side for negotiation even though it's used by the QUICSocket.

Ok so there's one crypto thing that QUICClient does require and that's the crypto.ops.randomBytes.

Ok so the problem is that if a socket is shared... then we have to know which connection the frame is supposed to go to.
In the Rust quiche examples this isn't being done, since every client example is using their own socket.
When the QUICSocket is shared, that means its actually already using a pre-set key from somewhere else. So when it is shared, the QUICClient only needs the crypto to randomly generate its own SCID at the beginning and nothing else.
However if the QUICClient is creating its own QUICSocket... the QUICSocket requires a crypto because it uses it to sign the dcid in order to derive the scid.
I think from the client's perspective should not be necessary since in that case the client and socket is 1 to 1, thus the packet is always intended for the client's connection.

Also for short frames, they drop the scid, and only the dcid exists now. The dcid  should however be equal to the client's own scid.

When a QUICConnection's connection fails, the destroy event is triggered due to timing out. The QUICClient has it's own destroy event, but that's only triggered when client.destroy() is called. But, the QUICConnection destroying doesn't trigger the QUICClient destroying.
Is it expected that we set up a destroy event handler to clean up the QUICClient? Can we have the QUICClient destroy itself when the underlying QUICConnection is destroyed?

So basically there are new events for this.

connectionDestroy
clientDestroy
serverDestroy

Events now bubble up always. However some classes "abstract" and wrap underlying events (they still bubble up) and are re-emitted as their own event.
So if a connectionDestroy occurs, then clientDestroy will also be emitted.
This is because in the QUICClient we would listen to connectionDestroy events and proceed to destroy the QUICClient too.
The "origin of change" can occur both ends. The user could call quicClient.destroy() and be a user-driven destroy. Alternatively the connection may be destroyed due to a timeout or if the server sent a CONNECTION_CLOSE frame, in which case it is a protocol-driven destroy, arising from the bottom of the stack.

I've changed the ConnectionErrorCode to use Uint8Array instead of Array<number>. That's more correct, since the error message is binary.

By enabling events from both sides, the user side and also the protocol side, this leads to two-way reactivity.
The tricky part is dealing with the concurrency here between these 2 events.
We could try serialising everything (React/Redux style with one-way reactivity), but that would slow things down.
For example in the QUICClient we may receive protocol events from both the QUICSocket and also the QUICConnection.
Suppose the QUICSocket is encapsulated and not shared.
Then both objects can emit socketError, socketStop, connectionError and connectionStop events.
In any of these 4 cases, it has to translate to a forced destruction of the QUICClient.
The force is important here because it propagates the undestroyed component. If the socket was stopped, we need to force stop the connection. If the connection was stopped, we need to force stop the socket.
The force therefore ensures that we don't assume that the other parts of the system is still working. Without it, attempting to stop the connection will make it want to do it gracefully, which means attempting to send the connection close frame on the socket. Which would fail, and end up resulting in an exception that bubbles back up the event handler that is orchestrating this reactivity. This will prevent the connection from being stopped and instead the QUIC client ends up emitting its own error. We don't want this to be the case, if the socket were to be errored or stopped, then we should expect that the QUIC client destruction and corresponding connection stop should work as intended.
The "multi-threading" of these concurrent 2 way reactivity is extremely tricky and easy to create concurrency bugs. Ideally we would have a better abstraction like the React/Redux style where we can serialise all event transitions to the model, but that seems also quite heavy for something that is supposed by very low level. Not sure if there are lightweight enough abstractions to enable all of this, or if system-level languages attempt to dispense with exceptions in all these cases, and instead result in a general panic (erlang style).

Small problem with QUICServer.stop and QUICClient.destroy. They both use the socket address in the logging messages.
    const address = utils.buildAddress(this.socket.host, this.socket.port);
    this.logger.info(`Stop ${this.constructor.name} on ${address}`);

The socket.host and socket.port are @ready decorated. So if we stop the QUICSocket before the client or server then this will result in a ErrorQUICSocketNotRunning error.
This is not a problem during normal usage, the socket should be stopped last. But for testing I'm stopping the socket to simulate the connection not responding. This is causing problems with tests and clean up.

We might do something where it is stopped it is still available.

What is the procedure for graceful stream close?
We now know that connections are just clsoed by sending a single CONNECTION_CLOSE frame and then timing out. There's no handshake necessary.
Then for streams, is this the case as well? Or do we instead need to do some sort of handshake?

For streams it's a little more complex. A stream is made up of two uni-directional streams that are made up of a sending state machine and receiving state machine.
A stream can close in three ways.

A fin frame is sent as the last message of the stream. This can be a 0-length stream.
Sending side shuts down the stream resulting in a STREAM_RESET frame being sent.
Receiving side shuts down the stream resulting in a STOP_SENDING frame being sent.

If the sending side shuts down the stream, the STREAM_RESET frame is sent. I think the receiving side will respond with a STOP_SENDING frame to acknowledge it. If the receiving side initiates the end it will send the STOP_SENDING frame, I don't think the sending side responds with a STREAM_RESET in this case.
It would be useful to have some tests to demonstrate this. The forward stream and reverse stream are symmetric. So we only need to demonstrate one direction.

So you can gracefully stop writing by sending a fin frame. But you cannot gracefully stop reading.
However you can explicitly force stop the writing with STREAM_RESET and force stop the reading with STOP_SENDING.

When we emit a QUICConnectionErrorEvent, right now the QUICClient will handle this and proceed to destroy the entire QUICClient.
Therefore any emission of error events is not simply informational, actions will be taken by the QUICClient and QUICServer.
For example if we do this.conn.recv, we may get TlsFail if we fail to verify the client.
In this case, it should not be an error event. This is because we will handle it gracefully and proceed to stop the connection.
However whoever is calling await conn.start() should eventually receive an exception indicating the error as to why it failed to start.
If this was the client, this would mean the client failed to verify the server's certificate or some other TLS failure.
In this case the client connection must gracefully handle this entirely.
And only at the very end will it resolve to throw an exception on the the start method.
However would it also emit an error event based on an exception?
Suppose the exception we expect is ErrorQUICConnectionTLS. Then we would expect:
try {
  await conn.start();
} catch(e) {
  if (e instanceof ErrorQUICConnectionTLS) {
    // ...
  }
}

That would be called inside QUICClient.createQUICClient and be propagated upwards.
Suppose we were to also emit a QUICConnectionErrorEvent, what would happen here? Is it possible that something catches this event and proceeds to try to stop the QUICConnection? In this case, it wouldn't really do anything since the QUICConnection.start failed and thus it would already be not running.

In the TLS tests, when there's a TlsFail, such as when the client fails verifying the server, a local error is acquired.
CLIENT LOCAL ERROR { isApp: false, errorCode: 304, reason: Uint8Array(0) [] }

This 304 code isn't part of the ConnectionErrorCode. This seems familiar @tegefaulkes?
Also when it is finally sent to the other side, we end up with a peer error.
SERVER PEER ERROR { isApp: false, errorCode: 304, reason: Uint8Array(0) [] }

With this, this means could translate these errors instead of using custom exception classes.

304 is one of the error codes I saw when debugging the tls problems with the generated certificates. Comment here https://github.com/MatrixAI/js-quic/issues/8#issuecomment-1510623836.
I haven't found a useful reference for what these codes are exactly. So beyond the TlsFail error they don't really add any more context.

This leads me to centralise all internal conn errors under:
/**
 * These errors arise from the internal quiche connection.
 * These can be local errors (as in the case of TLS verification failure).
 * Or they can be remote errors.
 * If the connection fails to verify the peer, it will be a local error.
 * The error code might be 304.
 */
class ErrorQUICConnectionInternal<T> extends ErrorQUICConnection<T> {
  static description = 'QUIC Connection internal conn error';
  public data: {
    type: 'local' | 'remote';
    isApp: boolean;
    errorCode: number;
  } & POJO;
}

I may want an actual exception message as well like TlsFail... cause it's interesting how you get this exception, but you still have to also go get the full error using conn.localError() or conn.peerError().

All TLS codes are listed here: https://github.com/google/boringssl/blob/master/include/openssl/ssl.h.
There's not much explanation. That's ok. They are all generally some stage of TLS failure.
This does mean that the ErrorQUICConnectionInternal needs to keep the error that comes out from quiche functions. It could be TlsFail or something else.
Right now I'm forcing the existence of the type, isApp, errorCode, reason.
The message like TlsFail could just be added to the message of the exception. Hopefully that just works.
I don't think we expect to see anything else.

Been thinking about concurrent state mutations and how to manage it well in js-quic. I ended up prototyping and releasing https://github.com/MatrixAI/js-async-monitor based on the discussion in https://github.com/MatrixAI/js-async-monitor/issues/1. This clarified some ideas for myself in terms of representing mutually exclusive atomic operations that I need to sequence with respect to the connection state. This makes sense at least for a single object, but I'm also dealing with multiple objects here, which creates a hierarchical concurrent object situation. A better model will probably arise eventually (something like petri-nets?).

On the plus side, I'm transitioning to use swc for jest transformer thus increasing the speed of our tests.

Ok one thing I realised after working on js-async-monitor is that I want re-entrant contexts similar to what I have already with DBTransaction. See issue comment https://github.com/MatrixAI/js-async-locks/issues/21#issuecomment-1575475316.
See in Polykey I relied on DBTransaction to provide me a transactional context that I can just propagate down the call stack to ensure re-entrancy.
But js-async-locks doesn't give me that easily. Primarily the reason here is that I'm not actually using a database. So while js-db gives me the DBTransaction for free, I would have to recreate a locking context here.
Now the way js-db does this is by having all operations run within the DBTransaction.
For example in TaskManager we can do await this.lockTask which just calls:
  protected async lockTask(tran: DBTransaction, taskId: TaskId): Promise<void> {
    return tran.lock([...this.tasksDbPath, taskId.toString()].join(''));
  }

And that just locks something in a re-entrant way.
To do such a thing in js-quic would require also a sort of locking context object.
Then instead of doing await this.connLock.withF, I have to instead do:
await locking.lock('somekey');

And pass this locking object down the call stack.
Of course the idea being that this is optional and create the context on the fly.
I discussed in js-async-monitor, that ensuring that this is the case is a bit complex because I have to enforce a certain parameter. It can be made more flexible using a parameter decorator.
It might look this:
@AsyncMonitor
class X {
  public f(ctx?: Partial<ContextLocked>): Promise<void>;
  @lock()
  public async f(@context ctx: ContextLocked): Promise<void> {
    // The decorator ensures that `ctx` is always fulfilled
    // But the outside signature indicates that `ctx` is optional and partial
  }
}

This is quite similar to how I use transactions atm in PK. Where the tran?: DBTransaction is the outside parameter type. Internally the function always acquires a transaction object if it wasn't passed in.
Now what is ContextLocked, it could be something like ctx.locks: LockBox. This special object I can then proceed to attempt to lock, and re-entrancy is enabled simply by checking if the a lock with that key already exists.
Anyway in the case of js-quic, my problem is that QUICConnection.recv should potentially call QUICConnection.stop. There are 2 cases where this can happen, either due to an exception during the loop that sends packets out, or due to the this.conn being closed at the end of the send loop. Usually this does not happen because the connections go through a period of time draining, but I cannot be sure here of what exactly quiche will do in all circumstances.

Now I don't actually want to use js-async-monitor in js-quic as I think it's a bit heavy weight here. But the same idea can be applied.
What I need is to change the signature to be like:
public async send(lock: Lock = new Lock()): Promise<void> {

Then create the lock if it doesn't exist.
If I want to do the above, I think I might add it into js-contexts with a ContextLocked type. That would mean you would have to combine these types together like ContextTimed & ContextLocked when you want to create something that uses multiple different possible decorators. Like:
@timedCancellable
@locked

Doing such a thing seems like js-async-monitor would be useless. Since you could just use @locked from js-contexts.
Huh...

While https://github.com/MatrixAI/js-async-locks/issues/21 discusses how to create a more generic version of these concurrency control constructs, we can take a page from js-db and just do a direct implementation of having a public async send(lock: Lock = new Lock()) parameter that just has the lock as an optional parameter along with public async stop(opts, lock: Lock = new Lock()).
As these are internal methods, it doesn't matter too much since users aren't supposed to be directly calling these anyway.

Actually doing the above doesn't fully solve the problem. There's nothing shared between the 2 locks. So instead maybe something like this?
public async send(lock: Lock = this.lock)
public async stop(opts, lock: Lock = this.lock)

That way by default, they would use a common lock...
The js-contexts package can solve this by having a common context... I think I remember saying that it should have a factory that takes in the context that is being shared between all decorators.

Based on this comment https://github.com/MatrixAI/js-async-locks/issues/21#issuecomment-1575562791, I cannot just do send(lock: Lock = this.connLock) I will also need a map that is being constructed on the fly book-keeping the keys that have been locked.
At that point, I may just go ahead and implement https://github.com/MatrixAI/js-async-locks/issues/21 and come back to js-quic.

In terms o the atomic operations here is what I'm thinking:
recv(ctx)
  - recv
  - if it is closed here
    - resolveClosedP
    - void stop.catch()
  - if it is draining
    - setTimeout
  - else
    - send

send(ctx)
  - send
  - if it is closed here
    - resolveClosedP
    - void stop.catch()
  - else
    - setTimeout

stop(ctx)
  - if closed
    - resolveClosedP
    - optional error
    - return
  - if draining
    - await closeP
  - else
    - close
    - send
    - await closeP

handleTimeout
  - create ctx
  - onTimeout
  - if closed
    - resolveClosedP
    - void stop.catch()
  - else
    - send

@tegefaulkes this has to be PR you have to apply the work to, there's too many differences from staging now.

Remember to remove the old certificate fixtures, since I wrote all the utility functions to create the proper certs and validated their behaviour, while the old certificate fixtures are likely to be incorrect.

I'm looking at this now. I want to get a clear picture of what still needs to be done.

The main thing right now is to apply mutual exclusion to QUICConenction handling of recv, send, close, etc etc... They will all share the same lock and pass a monitor down the call tree to allow re-entry with the locking.
I also need to transplant the secured event and custom TLS verification logic to this PR.
Old fixtures need to be removed and any usage should use the new generated certs.

I believe I already created promises for the secured event here, which is why you should working on this PR first.

There is a slight problem with awaiting the secureEstablishedP within the start method. The life cycle of async-init means its mutually exclusive with the stop method. But the stop method is the main way of cleaning up the connection and triggering events.
If there was an error with the connection such as the connection timing out. The background events wouldn't be able to call stop and cleaning up deadlocks.
I can think of two ways to deal with this.

Move awaiting the secureEstablishedP outside of the start method.
Duplicate the clean up logic between stop and an error during start.

I'll see how far I can get with 2. but there are some questions related to events. Do we emit an error and a stopping event when a connection fails during start? Or do we just rely on the start throwing for error propagation?

The start should throw an error. If an error occurs here then, no event should be emitted because we expect a user to catch it
Although if we want to be consistent we would emit an event if the instance exists. But we would not want to have this event do something like trigger a parent instance to then try to destroy.
So I think then start should just not emit the event since it bubble up the exception.
Mixing promises and events is always complicated we have to be clear what is a promise based control flow and what is an event based control flow. In the case of creation and start that's a promised based control flow. Once it is started though then we enter a event driven control flow.

@tegefaulkes I want to ensure that all the connection related logic is working first before moving to streams. That means the priority should be tasks: 8, 6, 7, 9, 11, 12. These are all interconnected so they have to be solved together.
Then subsequently on streams. Let's have a review tomorrow to see how our connection logic is going.

Ok, I was starting on the stream native tests but I'll switch focus in a moment.

Looks like @matrixai/timer Timer can't be repeated. If you call reset or refresh after it has settled it will throw. It may be a useful feature for @matrixai/timer to add a Interval for repeating timers.

I'm marking off the timer tasks. They were mostly done already. I only had to modify connTimeoutTimer to use reset() to update the delay.

I thought I could keep awaiting the secure established promise within the QUICConnection.start() method but there is a problem with that. It's technically already started and doing some background back and forth before  secure establishment. At any time it could fail and need to clean up, be it from a connection error or cancellation.
the QUICConnection.stop() method is the main way of handling clean up but as I mentioned before its mutually exclusive with start() and can't run at the same time. I thought I could add clean up logic to start if there was any failure but it's becoming too complex to have clean up logic in two places that can handle everything.  This has become more apparent when I was reviewing it in respect to ctx logic within start.
I'm going to have to move awaiting secure establishment outside of the start method. I'll likely have to make a createQuicConnection factory to contain this logic. That or replicate the steps within QUICClient and QUICServer.

Looks like @matrixai/timer Timer can't be repeated. If you call reset or refresh after it has settled it will throw. It may be a useful feature for @matrixai/timer to add a Interval for repeating timers.

In that case, just proceed to recreate a new timer if the timer is already exhausted. If it hasn't been exhausted we can reset it.

the QUICConnection.stop() method is the main way of handling clean up but as I mentioned before its mutually exclusive with start() and can't run at the same time. I thought I could add clean up logic to start if there was any failure but it's becoming too complex to have clean up logic in two places that can handle everything. This has become more apparent when I was reviewing it in respect to ctx logic within start.

It is possible for start and stop to be called within each other. However you generally don't want to do this. I think I had done an override. You can provide a status override that allows you call the stop function in the start while it is happening. See the @ready decorator. I think we used to do this at one point.
The question though is whether to merge stop behaviours into a single stop method, or to extract out some logic, because "stopping" can occur from user-driven method, or an event due to error.
We have a propagation of change problem too, that is if connection fails, we need to propagate failure to QUIC client and also QUIC Server and also streams.
The lifecycles has to be considered with respect to this reactivity issue:

Also for QUICSocket, it's lifecycle depends on whether its encapsulated or not.

In our framework here we have general principles. This is actually based on erlang OTP supervisor principles, you can learn about these here: https://www.erlang.org/doc/design_principles/sup_princ.html#spec

If the parent manages a collection of children, each child's death does not kill the parent. In this sense the parent is a supervisor.
If the parent is a 1 to 1 of the child, like in QUICClient, the child's death kills the parent. In this sense the parent is a 1 to 1 supervisor.
If a parent is killed, it always kills its children, assuming the children are encpasulated.
If the child is "adopted", then "ownership" is moved. We don't have this semantic in our programming generally unless explicitly indicated.
If the child is borrowed, then it is not encapsulated, and the parent dying does not affect the child. If the child dies, it may cause the parent to due depending on invariants.

For the start CTX do we want signalling abort to only abort the connection starting until secure establishement? Or can it abort the connection at any time? Similar to closing the connection but via the abort signal?

Based on the discussion about timeouts.
We want to lean on the quiche internal timeouts but still have the option for 0 timeout meaning no timing out the connection. We want to default the timeout to 1 min.
We need to add a constraint to the start and keep-alive times to be strictly less than the idle timeout. but ignore this constraint if the timeout is set to 0.
So we can have an infinite timeout by setting the maxIdleTimeout to 0. But there is no convenient way to know that the connection has failed. It may have it's uses so we'll allow it.

Describe the life-cycle and object map handling for the client/server -> connection -> stream. Outline how events flow from top down and bottom up. Give examples.
There are two directions events can flow. Top down, where a parent will trigger the destruction of something. And bottom up, where a internal concurrent event will trigger it.
Top down control works through function calls and follows the normal call stack. So for example, a QUICConnection can be destroyed by calling connection.stop() with a user/app error or a parent such as QUICClient or QUICServer can call connection.stop during it's own life-cycle.
Bottom up flows through events and event emitters. The destruction of a QUICStream will trigger a streamDestroy event on its emitter. Any kind of destruction, i.e. from calling stream.destroy() or from internal errors will result in this event.
Life-cycle and object map handling can be triggered via these events.
For top down flow should always function as such, destruction is called via stop() or destroy(). This triggers destruction of any children, e.g. connection destroying any active streams. And completes any further clean up such as closing the connection and emitting events.
Bottom up flow is handled by hooking onto an event. For example, the object maps need to remove anything that has finished. Such as the QUICConnection removing any finished QUICStreams. This stream map will be fully managed by the QUICConnection object. A stream destroy event will trigger the removal of that stream from the internal map. Likewise a connection destroy event will trigger the removal of that connection from the server's connection map. The client will trigger it's own destruction from a connections destroy event.
Destruction of a object should always be handled via it's stop() or destroy() method. This can be called internally or via its parent. These methods should handle all clean up such as propagating destruction downwards and awaiting children to clean up. But also propagate events upwards such as the destruction event. These methods should only resolve once all state and children has cleaned up. Where applicable they have a force option that will initiate clean up of children or await for them to finish normally.
Examples
For the QUICStream, top down is triggered via calling destroy(). this will force close any open streams with an error, this can only be an error. It will then await for the stream to fully end and the internal quiche state to clean up before resolving and emitting the destroy event.
for bottom up destruction, the destroy is triggered if both the readable and writable has ended, with or without errors. Any errors are thrown through the streams themselves.
For the QUICConnection, top down is triggered via calling stop(), if forced, this will trigger the destruction of any active streams and wait for them to end. Otherwise it will just wait for them to end. it will then trigger connection closing and wait for the connection to resolve. If the connection timed out or had a localError or peerError it will emit these and the stop event and then resolve.
Bottom up is triggered by the quiche connection closing calling stop(), with or without error. Removal of a stream from the stream map is triggered by that connection's 'destroy' event.

Please update the diagrams and repost it with the above represented.

Based on the discussion about timeouts.
We want to lean on the quiche internal timeouts but still have the option for 0 timeout meaning no timing out the connection. We want to default the timeout to 1 min.
We need to add a constraint to the start and keep-alive times to be strictly less than the idle timeout. but ignore this constraint if the timeout is set to 0.
So we can have an infinite timeout by setting the maxIdleTimeout to 0. But there is no convenient way to know that the connection has failed. It may have it's uses so we'll allow it.

Add a task above to track this.

Diagram pending.

Note that the connection map is actually owned by QUICSocket. We can consider the QUICClient, QUICServer and QUICSocket are all "friend" classes so they can mutate each other's internal state. So that way QUICClient and QUICServer could be the one that actually deals with the connection map.
If we are changing this, and you have a diagram ready representing all of this. We may wish to replicate this idea back to PK.

Still need to do .9, .12 and .13 for connection level changes. This includes transplanting the custom TLS tests and fixing up the QUICServer tests. Then I can move on to the QUICStream level stuff.

Here's a diagram of the connection ID negotiation that I derived from what was on the whiteboard:

It's an embedded with excalidraw drawing information so we can later update it.
This should then be part of the documentation of js-quic. All into the README.md...

notes
Oddities that I have noticed.
There are a few ways a stream can finish, isFinished() becomes true with the following conditions

Stream finishes with fin frame

If there is no buffered data to read, isFinished will be true. but will still be listed as readable in the conn.readable(), conn.isReadable() and conn.streamReadable() until it is read. Reading will yield no data but fin will be true. Afterward it will cease to be readable and reads will result in done.
If there is buffered data all data must be read before isFinished becomes true. After reading all buffered data, fin flag will be true, is finished becomes true and the stream ceases to be readable.

shutdown by sender with streamShutdown(write)

If there is no buffered data, the stream will still be readable and in the iterator. isFinished() will be immediately true. Reading will result in a StreamReset(code) error and the stream will cease to be readable.
With buffered data to read, the stream will be readable and in the iterator. isFinished() will be true. Reading will result in a StreamReset(code) error and any data that was buffered is just lost. after reading the stream will cease to be readable.

shutdown by receiver with streamShutdown(read)

Stream does not become readable, and reading throws Done, isFinished() is still false. Sender needs to process a packet and respond. Afterward any reads results in a StreamStopped(code) error and isFinished() is true. Note that the stream does not become readable during this.
With buffered data, all buffered data is dropped, and the stream ceases to be readable. isFinished() is STILL false and any reads will return Done. Sender needs to process a packet and respond. Afterward any reads results in a StreamStopped(code) error and isFinished() is true. Note that the stream does not become readable during this.

Shutting down the readable stream with streamShutdown(Read) is a little odd compared to the other cases. It needs to wait for a packet to be sent and received before the stream actually finished. It's also odd that the stream immediately ceases to be readable and never becomes readable again to read out the error. I suppose you don't need to read it since you specified the error, but you still need to wait for it to become finished. but it won't appear to be readable even when it finished and reading after its finished will throw Done.
We need to be EXTRA careful, if the connection has closed before isFinished() becomes true then it will never become true. I should double-check if we even need to wait for isFinished() to be true before cleaning up the QUICStream state. Actually yeah, if you shut down the readable, just ignore it afterward unless there is any explicit timing reason with clean up to check it.
Other things to note. generally isFinished() is false until all buffered data has been read. Except for when there is an error. In that case isFinished() is true immediately. So if isFinished() becomes true, we must read out all remaining data which is either an error of an empty message, and handle closing the QUICStream readable stream.

Given the above, I think we can avoid the double buffering on the readable stream with the following condition. if the stream finishes with isFinished() becoming true while the readable is blocked, we need to ignore the blocking and read that stream to either get the finish frame, or an error message that we can close/error the readable stream with.

Some notes about the writable stream behaviour.

finishing the stream with a fin flag

When closing the writable with a 0-len fin frame, the stream is no longer in the writable iterator. streamCapacity and StreamWritable still indicates that it is writable however. Any further writes to it will result in a FinalSize error being thrown. After the writable and readable have finished, streamCapacity and StreamWritable will throw InvalidStreamState(streamId) and any further sends will throw Done.
Same is true for streams with buffered data.

Shutting down stream from sending side with streamShutdown(write)

After shutting down with streamShutdown(write), any further shutdowns will result in Done. streamWritable and streamCapacity will show it's still readable. The stream will not be in the writable iterator. Any Buffered data will be dropped. Any further writes throw FinalSize.

Shutting down stream from receiving side with streamShutdown(read)

After the peer has shutdown the readable and sent the packet indicating this. The streamWritable and streamCapacity will throw StreamStopped(code). The stream will be indicated as writable and in the writable iterator. Any further writes will throw StreamStopped(code), after writing the stream will no longer be in the writable iterator. After the stream has fully finished writable and readable, further writes will result in Done.

Some notes about quicStream.
In terms of life cycle. The writable stream only closes in 3 ways. The graceful case of closing the stream with a fin flag. and the two error cases where the stream is shutdown with streamShutdown(writable) from the sending side, or streamShutdown(Readable) from the receiving side.
When the stream is closed with a fin flag during send. That's it, the writable is done. The stream will never be in the writable iterator. We don't have to wait for a response or state change, we can just end the writable webstream.
When using streamShutdown(writable), the same is true, it will never be in the writable iterator, any further writes result in FinalSize. Pretty much identically handled as the first case.
The 3rd case where the peer shuts down the stream with streamShutdown(readable) needs some extra care. There will be two ways to tell it has ended. The first is it will throw StreamStopped(code) if you write to it but since that depends on app input. streamWritable and streamCapacity will throw StreamStopped(code) as well. so we just need to check streamWritable(0, 0) after receiving a packet.
So the writable webstream is handled in the following way.

We configure it with a 1 message buffer, Buffering will be handled by the quiche layer.
Writing into it will pass the data to the stream with streamSend(id, data, finFlag).
Closing the stream will close the quiche stream with streamSend(id, new Uint8Array(), finFlag). We may include some data here to fix a potential bug with 0-len finish messages. The quiche writable should be treated as ended at this point.
If the webstream is aborted or cancelled. we shut down the quiche stream with an error with streamShutdown(id, write, code). The quiche writable should be treated as ended at this point.
If writing to the quiche stream results in an error StreamStopped(code) then error out the writable webstream and treat the writable stream as closed.
If streamWritable(0, 0) results in an error while processing the writable iterator. then error out the writable webstream, write a dummy message with `streamSend(id, message, false) so the stream is removed from the writable iterator and treat the writable stream as closed.

This should cover all termination conditions for the writable stream.

In terms of the readable stream, the readable can close in 3 conditions. A graceful end when receiving a fin flag when reading data. And the two error cases where the readable is closed locally with streamShutdown(read) and closed remotely with streamShutdown(write).
If a stream is closed with a fin flag, the stream does not end until we read all of the data out. If there was no buffered data then streamFinished() will be true but it will still be readable until we read out the fin flag. So for a graceful end, we must read all buffered data until we see a fin flag. Then the stream is considered ended and we can end the readable webstream.
When the stream is remotely closed using streamShutdown(write). All buffered data will be dropped. streamFinished() becomes true even if there was buffered data. the stream will still be in the readable iterator. The next read will throw the error code and remove it from the readable iterator.
If the stream is locally closed with streamShutdown(read) then all buffered data is dropped and the stream is removed from the readable iterator. streamFinished() will return true after some communication but the stream will never show on the readable iterator. Unless we really need to know that the peer has processed the close, we don't need to wait for the streamFinished() to update. We can consider the readable ended and close the readable webstream with an error.
So the readable stream is handled in the following way.

We configure the stream with a 1 message buffer as buffering is handled on the quiche layer. It's possible to use a BYOB stream here, could look into it.
We pull data by checking if the stream is readable with streamReadable(id) and reading out some data. Most data will be obtained in a pull style.
If we read and fin flag is true, we close the readable stream gracefully. the stream is considered as done.
If we read the quiche stream and it throws, we close the webstream with an error. The stream is considered as done.
If the stream is in the readable iterator AND streamFinished() returns true. then we must immediately read the stream and process it's closed. It could be a fin frame with no data so we close the webstream gracefully. OR an error code so we close the webstream with an error.

Oh by the way, it doesn't seem possible to re-dispatch an event. you can create an event a dispatch it multiple times. But we can't take an emitted event and re-dispatch it. Doing so results in an error like this. Error: The event "connectionStream" is already being dispatched
So we'll have to re-create the event. with the details.

You can't redispatch on the same event target, but you should be able to redispatch on a parent event target or a different event target right?

Has all the TLS cert fixtures been deleted? We don't need them anymore since I created all the generate utilities before.

The changes to QUICStream are mostly done and most of the tests are working now. There are some small things to iron out such as propagating remoteInfo. I need to do a little bit of cleaning up of the stream logic in QUICConnection.
Part of the changes to QUICStream is that I converted the push based logic for the readable stream to a pull based system. If it pulls and there is no data, it will wait for the stream to become readable again. When the stream is in the readable iterator it will resume again.
I just realised there's a bug in my logic and I need to read the stream and process any errors outside of the streams pull method if it is finished. Otherwise it will still block on buffered data.
The streams have been changed to only buffer a single message. Buffering is left to the quiche logic to handle.
I hopefully should be able to complete the stream level changes within a day. Barring any major bugs that crop up.

Nope, the events are connection level events being re-emitted on the client emitter but they are throwing. I'm guessing it's tracked by the event itself.
I've completely removed the fixtures and have been using the generate function.

Ok, for the readable stream there were two cases were it would block. When there is no data to read, or when there is already buffered data in the web-stream.
In the first case. when the readable stream does a pull and there is no buffered data in the quiche stream. It will wait on a promise that will get resolved when the stream is in the readable iterator.
The second case will not attempt a pull to read the quiche stream if it already has buffered a message. This is fine during the graceful case, but if the stream was closed with an error we need to respond to that. So if the stream is in the readable iterator But streamFinished() is true then the stream has closed with no further data to read. Either with a fin frame or error. So we can read and process the stream close in that case.
I have created two new tests to check these two conditions.

Can you also test connection kills when streams are still in operation. So that abnormal terminations.

Yep, there are 3 tests for this in the stream tests.

local connection closed,
peer connection closed,
connection timed out

At some point the connection.remoteInfo getter was removed. It was used by the QUICStream to propagate connection details. I don't know why it was removed? we still kind of need it so I need to re-implement it but I'm not sure what the desired method is.

I think it should be a function not a property. It's something that only exist after being started and should have the ready decorator applied.

Getters can have a @ready applied to them. I'd be fine with a function but I think the agnosticRPC expects the connection data in the form of a property of the object and not a function.

Should be ready for clean up and review soon.

I'm just working on some final clean up and fixes. Right now I'm trying to work out why the process is being held open after tests have finished.
There are only two things that can hold the process open. any active sockets or any active timers. I'm thinking there's an active timer somewhere.
incidentally, you can use console.log(process.getActiveResourcesInfo()) to list anything active in the event loop.

I fixed up some logic for cleaning up connections when a socket fails or stops.
I also found what was keeping the process open. It was a timer used to timeout a condition within a test.
    await Promise.race([
      streamEndedProm.p,
      testsUtils.sleep(200).then(() => {
        throw Error('Ending timed out');
      }),
    ]);

This is fine for short timeouts but the timer isn't cleaned up or dereference it in any way. so it will hold the process open. I had modified one of these timers to have a delay of 100000 to essentially disable it. I hadn't fixed up so it was holding the process open after all the tests complete. Just something to note for the future.

I fixed up some logic for cleaning up connections when a socket fails or stops.
I also found what was keeping the process open. It was a timer used to timeout a condition within a test.
    await Promise.race([
      streamEndedProm.p,
      testsUtils.sleep(200).then(() => {
        throw Error('Ending timed out');
      }),
    ]);

This is fine for short timeouts but the timer isn't cleaned up or dereference it in any way. so it will hold the process open. I had modified one of these timers to have a delay of 100000 to essentially disable it. I hadn't fixed up so it was holding the process open after all the tests complete. Just something to note for the future.

There must be a more robust way of doing this...?

I could make a utility function to do this and handle clean up. something like resolveWithin(promise, delay).

I meant in general tests should depend on a specific amount of time anywhere.

Because as you know. Wall clock time is not deterministic. Computers can be fast or slow.

Right now we're not propagating any stream events. It doesn't really have any events besides starting, which is not a stream level event. And stopping. Do we want the quic connection to propagate that event? would that even be useful? we'd need some way to indicate which stream has stopped. Should the event contain the stream object?

I meant in general tests should depend on a specific amount of time anywhere.

True, they only exist so tests don't take a really long time to fail, but also indicate where in the test was timing out. There can be several parts of a test where we wait for something.

Right now we're not propagating any stream events. It doesn't really have any events besides starting, which is not a stream level event. And stopping. Do we want the quic connection to propagate that event? would that even be useful? we'd need some way to indicate which stream has stopped. Should the event contain the stream object?

So for this I planned that each event should be emitted with a specific name like streamStop or connectionStop. Further distinction on which connection stream should be done by inspecting the emitted event data.
Generally speaking all events should propagate upwards simply for information reasons. That doesn't mean the user would actually listen on it. This is to avoid having the user have to always bind event listeners to the sub-objects.

Fixed event propagation. There is still some review and clean up I need to do for streams, server client and socket.

For the stream it may be useful to have promises that resolve at certain events. Such as recvClosedP, sendClosedP and destroyedP.  But I suppose I could construct a promise from events but having a ready made promise is much cleaner.

I'll start re-basing now.

For the stream it may be useful to have promises that resolve at certain events. Such as recvClosedP, sendClosedP and destroyedP. But I suppose I could construct a promise from events but having a ready made promise is much cleaner.

Well that's what we've been doing right? We're basically mapping events to promises. It's a push to pull conversion.

@tegefaulkes make sure to also resource lifecycle the async monitor instance.
I created a utility function for this.
/**
 * Monitor resource.
 * Use it with `withF` or `withG`.
 */
function monitor<RWLock extends RWLockReader | RWLockWriter>(
  lockBox: LockBox<RWLock>,
  lockConstructor: new () => RWLock,
  locksPending?: Map<string, { count: number }>,
): ResourceAcquire<Monitor<RWLock>> {
  return async () => {
    const monitor = new Monitor<RWLock>(
      lockBox,
      lockConstructor,
      locksPending
    );
    return [
      async () => {
        await monitor.unlockAll();
      },
      monitor
    ];
  };
}

Here is it. It's currently in https://github.com/MatrixAI/js-contexts/pull/2.

I've fixed up the Monitor logic to clean them up when done.

I'm merging this now. We'll do a proper review in staging.

Great!