Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
addievo opened this issue Sep 28, 2023 · 12 comments · Fixed by #42
Closed

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

addievo opened this issue Sep 28, 2023 · 12 comments · Fixed by #42
Assignees
Labels
development Standard development

Comments

@addievo
Copy link
Contributor

addievo commented Sep 28, 2023

Specification

Factor out the timeout middleware.

Tasks

    1. Move timeout middleware into js-rpc.
    1. Move the rpc request and response types to js-rpc. Replace the ClientRPCRequestParams as an extension of the default request and response types. You can create a more specific subtype when using extends here. This applies also to AgentRPCRequestParams and associated.
    1. Timeout middleware must be applied to the agent service as well.
@addievo addievo added the development Standard development label Sep 28, 2023
@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 12, 2023

I believe the timeoutMiddleware that is currently PK's src/client should actually be part the default middleware, since it is a core function of js-rpc.

https://github.com/MatrixAI/Polykey/blob/staging/src/client/timeoutMiddleware.ts

This should really be in js-rpc.

I think this (https://github.com/MatrixAI/Polykey/blob/staging/src/client/types.ts):

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

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

This can be part of the js-rpc library as well. The only thing imposition here is simply the fact that js-rpc says we need to reserve the constraint property in the request and response messages.

@CMCDragonkai
Copy link
Member

If you want to make it flexible it can be parameterised as the "reserved key".

@CMCDragonkai
Copy link
Member

ATM the agent service is not using the timeout middleware at all, and it should be. So there needs to be a fix here too.

@CMCDragonkai
Copy link
Member

You can work on this @addievo and start a PR here and a PR in Polykey, they should be kept in sync.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 12, 2023

Note that this is incorrect:

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

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

The authorization for agent service cannot happen, it's always a public service.

@CMCDragonkai
Copy link
Member

This is part of #551, and we want to make sure this is ready by mid next week.

@addievo
Copy link
Contributor Author

addievo commented Oct 12, 2023

Alright, just imported ClientRPCRequestParams and ClientRPCResponseResult and exported them to use in middleware, thats it for js-rpc.

  1. Copied over the timeoutMiddlewareClient and timeoutMiddlewareServer functions to js-rpc src/middleware
  2. To be implemented in default middleware.

@CMCDragonkai
Copy link
Member

They should not have the Client prefix anymore. It is generic now.

@CMCDragonkai
Copy link
Member

If you want to make it flexible it can be parameterised as the "reserved key".

Remember this too.

@addievo
Copy link
Contributor Author

addievo commented Oct 12, 2023

Timeout middleware on chunk.params doesn't make much sense in rpc context as in PK we assumed all values to be a valid JSON, but in RPC they aren't, consequently, while the middleware is now working in rpc, many jests are failing due to the same. And therefore it would make more sense on chunk

@CMCDragonkai
Copy link
Member

I don't know what you mean.

@tegefaulkes
Copy link
Contributor

Allow users to extend or modify middleware while also keeping the default functionality. This is already a feature, middleware is provided by the user through the middlewareFactory parameter. You compose it with the default by using the utility functions.

@CMCDragonkai CMCDragonkai changed the title Middleware Composition and Staging in js-rpc Factoring out the Timeout Middleware from PK to js-rpc Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
3 participants