Feat monitor fixes

[tegefaulkes]

Description

This PR addresses some small issues in the js-quic code.

As per discussion, the constraint on the connection start timeout should be removed. During usage we can set this to whatever without the context of the idle timeout being know. In that case it's very likely to throw an error. We'll allow any value to be valid with the caveat that a lower idle timeout will supersede the startup timeout.

During review we found that server type connections startup timeout defaults to infinity. creation here depends on establishment and secure events so it's possible for a server connection to be stuck in limbo if these events are never reached. A default start timeout should be applied here.

Issues Fixed

• Fixes Monitor usage in QUICConnection #48

Tasks

• 1. Address issue Monitor usage in QUICConnection #48
• 2. Apply startup timeout to a server connection getting created
• 3. Remove constraint between start timeout and max idle timeout values. A longer startup timeout will be allowed but superseded by the idle timeout.

Final checklist

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

[ghost]

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

Legend

[CMCDragonkai]

Also move the recv and send into the start method of QUICConnection.

[tegefaulkes]

as per task 2. we need a startup timeout for server connections. This is for the connection to establish and secure before it's fully created as a QUICConnection. This needs to be a new option for the QUICServer to set and have a default.

I'm not sure what to call this new option. connectionEstablishTimeoutTime, connectionCreationTimeoutTime, connectionStartTimeoutTime, connectionConnectTime?

What's a reasonable default here? Technically the connection is already created so we're just waiting for some negotiation. So 1-2 seconds is more than enough?

[CMCDragonkai]

No I already specced this out earlier in the configuration entropy issue.

connectTimeoutTime
keepAliveTimeoutTime
keepAliveIntervalTime

Is it possible to do a constraint check if one were to create this?

Also did you check if it's possible to mutate the idle timeout afterwards?

[CMCDragonkai]

Again defaults based what I talked about yesterday.

15 seconds for connect, 30 seconds for keep alive, 10 seconds for interval.

Make this consistent everybody. Already in my PR.

[tegefaulkes]

For reference

# Polykey

* nodesConnectionConnectTimeoutTime: 15000,
* nodesConnectionKeepAliveTimeoutTime: 30000,
* nodesConnectionKeepAliveIntervalTime: 10000

## Client

QUICClient.createQUICClient(
  {
    config: {
      connectTimeoutTime: 15000,
      keepAliveTimeoutTime: 30000,
    }
  },
  {
    timer: 1000
  }
);

So quiche's `maxIdleTimeout` is encapsulated in our `js-quic`.

It also cannot be mutated.

It serves as both the connect timeout and the keep alive timeout for the underlying quiche connection.

In order to get the behaviour we want.

1. `maxIdleTimeout` would need to be set the maximum of `connectTimeoutTime` and `keepAliveTimeoutTime`.
2. The reason is because the `timer` is intended to mean how long I'm going to wait for the function to return, otherwise abort.
3. ? `timer <= connectTimeoutTime`
4. ? `connectTimeoutTime > keepAliveTimeoutTime` - Throw an error. This is not possible atm in js-quic.
5. If the `connectTimeoutTime <= keepAliveTimeoutTime` - We have to do a JS-level timer and abort.

Now let's change this up a bit.

config: {
  connectTimeoutTime: 15000,
  keepAliveTimeoutTime: 30000,
}

@timedCancellable(true, connectTimeoutTime, ...)
QUICClient.createQUICClient(
  {
    config: {
      keepAliveTimeoutTime: 30000, // this overrides the `keepAliveTimeoutTime`
    }
  },
  {
    timer: 1000 // this overrides the `connectTimeoutTime`
  }
);

1. Do we want connect timeout time to default to Infinity for both client and server? If so, then on `QUICClient`, you use `Infinity` for `@timedCancellable`, and secondly on `QUICServer` you just don't do anything.
   - Answer: This is moot. Because of the `maxIdleTimeout`. Unless you set the `maxIdleTimeout` to `0`, it's not possible to have an infinity server and client timeout.
2. The alternative is that both client and server have a default connect timeout of 15 seconds.


`connectTimeoutTime <= keepAliveTimeoutTime` is necessary.

But you cannot apply to the `timer`.

The end result is:

1. `keepAliveTimeoutTime` is the max boundary on both connect timeout and keep alive timeout
2. `timer` is the min boundary on only connec timeout

Because we aren't representing the true semantics of all of this

/**
 * Usually we would create separate timeouts for connecting vs idling.
 * Unfortunately quiche only has 1 config option that controls both.
 * And it is not possible to mutate this option after connecting.
 * Therefore, this option is just a way to set a shorter connecting timeout
 * compared to the idling timeout.
 * If this is the larger than the `maxIdleTimeout` (remember that `0` is `Infinity`) for `maxIdleTimeout`, then this has no effect.
 * This only has an effect if this is set to a number less than `maxIdleTimeout`.
 * Thus it is the "minimum boundary" of the timeout during connecting.
 * While the `maxIdleTimeout` is still the "maximum boundary" during connectin.
 */
const minIdleTimeout = Infinity,

export { minIdleTimeout };


config: {
  // Synthetic parameter, it is the minimum boundary, if this is greater than max idle timeout, it has no effect
  // It only has an effect if this is lower than the max idle timeout
  // On the client side it is the default for the timed cancellable decorator
  // On the server side, it is the default on `ctx.timer` when calling `QUICConnection.createQUICConnection`


  // This is the maximum boundary for connect timeout and keep alive timeout
  // Default to 0 to have a consistent defaults of "libraries" always using infinity
  maxIdleTimeout: 0,

  // Synthetic parameter (default undefined means no keep alive)
  keepAliveIntervalTime: undefined
}

@timedCancellable(true, minIdleTimeout, ...)
QUICClient.createQUICClient(
  {
    config: {
      maxIdleTimeout: 5000, // this overrides `maxIdleTimeout`
    }
  },
  {
    timer: 1000 // THIS IS THE MIN BOUNDARY
  }
);

## Server

Uses `Infinity` - which basically means the way it was before.


new QUICServer({ 
  config: {},
  minIdleTimeout
});



QUICConnection.createQUICConnection({}, { timer: minIdleTimeout })


Make sure the `QUICConnetion` is still defaulting to `Infinity`. No more constraints on `ctx.timer`.

[tegefaulkes]

Everything is addressed now. Waiting for the CI to finish to merge.

[CMCDragonkai]

Can you tick off tasks and the final checklist?