feat: incorporate timeoutMiddleware to allow for server to utilise client's caller timeout

[addievo]

Description

Timeout Middleware

This PR moves timeoutMiddleware from Polykey to js-rpc. The middleware allows for the RPCServer to lessen it's timeout based on the advisory timeout set by the client. This has been decided to be moved across to js-rpc, as realistically it should be a core feature.

The metadata type will have to be moved across as well, but with the authentication property removed, as that's not in the scope of the timeoutMiddleware.

// Prevent overwriting the metadata type with `Omit<>`
type JSONRPCRequestMetadata<T extends Record<string, JSONValue> = ObjectEmpty> =
  {
    metadata?: {
      [Key: string]: JSONValue;
    } & Partial<{
      timeout: number | null;
    }>;
  } & Omit<T, 'metadata'>;

// Prevent overwriting the metadata type with `Omit<>`
type JSONRPCResponseMetadata<
  T extends Record<string, JSONValue> = ObjectEmpty,
> = {
  metadata?: {
    [Key: string]: JSONValue;
  } & Partial<{
    timeout: number | null;
  }>;
} & Omit<T, 'metadata'>;

Optional Timeout Throwing for Handlers

It was found that timing out would automatically throw an error from js-timer. This was because it was not checked whether the Timer instances had been settled or not before timer.reset() and timer.refresh() had been called. This has now been fixed, meaning that ErrorRPCTimedOut is now propagated to ctx.signal, and can be optionally thrown by the handler as intended.

Issues Fixed

• Fixes Factoring out the Timeout Middleware from PK to js-rpc #19

Tasks

• 1. Move relevant types from PK
• 2. Import timeoutMiddlewareClient and timeoutMiddlewareServer from PK src/client/timeoutMiddleware.
• 3. Implement timeoutMiddleware in defaultMiddleware.
• 4. Provide spec on the timeout behavior on the README.md - so it can be consistent
• 5. On the server handlers, if the timeout has occurred, the handlers need to be able to ignore it, and continue executing, there shouldn't be a timeout error - the client call may have already thrown
• 6. The serialisation of Infinity is null, therefore timeout has to be null | number on the JSON RPC message type metadata.

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]

This PR name is too specific, make sure to consider all of the issue scope.

[addievo]

Ready for review

[CMCDragonkai]

Rebase on staging.

And it's ready for review... not ready for merge.

[addievo]

Rebased on staging

[CMCDragonkai]

Open up a co-PR on PK that assumes this is being integrated. I need to see the whole context.

[tegefaulkes]

@CMCDragonkai I need more context about the spec before I can do a full review.

I recall discussing that we'd move the metadata out of the params and response fields and merge it into the JSONRPC message structure as optional parameters. Is that still a requirement?

For example, the JSONRPCRequestMessage becomes something like this?

// old
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
};

// new
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
  metadata?: {
    timeout?: number | null;
  } & Omit<Record<string, JSONValue>, 'timeout'>;
};

[CMCDragonkai]

@CMCDragonkai I need more context about the spec before I can do a full review.

I recall discussing that we'd move the metadata out of the params and response fields and merge it into the JSONRPC message structure as optional parameters. Is that still a requirement?

For example, the JSONRPCRequestMessage becomes something like this?

// old
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
};

// new
type JSONRPCRequestMessage<T extends JSONValue = JSONValue> = {
  jsonrpc: '2.0';
  method: string;
  params?: T;
  id: string | number | null;
  metadata?: {
    timeout?: number | null;
  } & Omit<Record<string, JSONValue>, 'timeout'>;
};

What's the trade off here? Is it just that we leave the json RPC spec? Maybe it's necessary.

[tegefaulkes]

I think it's needed. It's a hard requirement that we can't impose structure on the params and response of the messages. These are purely application defined. So if we have default timeout middleware that requires a timeout parameter in the message, it can't be part of the application supplied data.

But this has other consequences, if metadata is no longer supplied as part of the input and output of the caller and handler, then how does the caller and handler access or set the metadata now?

If we DO impose structure on the params and result then they can't be JSONValue anymore. It needs to be a Record<string, JSONValue> and we need to enforce types on the metadata and required parameters while preventing the application from overwriting their types. That may be easier to implement since there will be less changes to the API. But if we're using the RPC from the 3rd party perspective, the structure of the params will need to be know and that has usability issues since we're still diverging from the JSONRPC spec.

In either case there is more work to be done here.

[tegefaulkes]

Based on discussion, we're going to expand on the params and result parameter types to include the metadata and timeout. This has the least required changes to make this work.

we need to modify the JSONRPC message types to reflect this.

I'd have to fully review the changes to know what's still needed. But the following needs to change.

1. JSONRPC messages are updated so the at the RPCRequestParams<T> is used for the params type, and the RPCResponseResult<T> is used for the result type. The T is now T extends Record<string, JSONValue> = ObjectEmpty.
2. Tests need to be updated to reflect this.
3. Need to check that any arbitrary metadata can be provided, but the timeout type can't be overwritten.

[CMCDragonkai]

This will come after #47, and @amydevs can take over this to update the JSON RPC types, and factor out the timeout middleware.

[amydevs]

Should be ready to review, still need to update description, cos i pushed a few unrelated fixes regarding another issue

[CMCDragonkai]

@amydevs tasks on OP spec need to be covered.

[CMCDragonkai]

@amydevs can you also explain what also needs to be done on PK as well to support this change? What will need to be removed. Is there an existing issue in MatrixAI/Polykey-CLI#40?