Update handler and caller timeouts to be able to overwrite server and client default timeouts regardless of value. #47

addievo · 2023-10-20T04:23:22Z

Description

Previously Handler.timeout could only overwrite the default timeout if it was lesser than the default timeout, this is problematic in instances where we need a greater timeout than default timeout as in

MatrixAI/Polykey#589 (comment)

The previous implementation of timeout was as so:

if (handler == null) {
        await cleanUp(new errors.ErrorRPCHandlerFailed('Missing handler'));
        return;
      }
      if (abortController.signal.aborted) {
        await cleanUp(
          new errors.ErrorHandlerAborted('Aborted', {
            cause: new errors.ErrorHandlerAborted(),
          }),
        );
        return;
      }
      // Setting up Timeout logic
      const timeout = this.defaultTimeoutMap.get(method);
      if (timeout != null && timeout < this.handlerTimeoutTime) {
        // Reset timeout with new delay if it is less than the default
        timer.reset(timeout);
      } else {
        // Otherwise refresh
        timer.refresh();
}

This PR aims to override this by allowing Handler.timeout to have priority over normal timeout irrespective of value, this is implemented as so.

     const timeout = this.defaultTimeoutMap.get(method);
      if (timeout != null) {
        // Reset handlerTimeout with new delay if it is less than the default
        timer.reset(timeout);
      } else {
        // Otherwise refresh
        timer.refresh();
      }

With this we hope to be able to have custom timeout for handlers even if we need to extend the timeout to be more than the default value.

Further, this PR aims to write tests to test this functionality, these tests are simply structured to have a handler timeout, which can be either greater or lesser than the default server timeout, the handlerTimeoutTime, and then to check if the updated timeout is set as the ctx timer.

These tests can be structured as so:

class TestMethodLongTimeout extends UnaryHandler {
        // This will override the default timeout from 50 to 250, thereby increasing it.
        timeout = 250;
        public handle = async (
          input: JSONValue,
          _cancel,
          _meta,
          ctx_,
        ): Promise<JSONValue> => {
          ctxLongProm.resolveP(ctx_);
          await waitProm.p;
          return input;
        };
      }
      const rpcServer = new RPCServer({
        handlerTimeoutTime: 50,
CMCDragonkai marked this conversation as resolved.
        logger,
        idGen,
      });
      await rpcServer.start({
        manifest: {
          testLong: new TestMethodLongTimeout({}),
        },
      });

In the aforementioned test, the server timeout was set to 50, and the Handler timeout to 250, consequently the final ctx timer should be 250, and this is expected.

Similar test for a lesser timeout is conducted.

Although the functionality of setting Client timeout through caller was already present, the tests to check the same were lacking, this PR also aims to include such tests. These tests would be structured similarly to the server tests wherein a client is created with a streamKeepAliveTimeoutTime and then the caller sets a different timeout, either greater or lesser, and we expect the ctx timer to be set according to the caller set timer, which can be expected as such:

        const rpcClient = new RPCClient({
          manifest: {},
          streamFactory: async (ctx) => {
            ctxProm.resolveP(ctx);
            return streamPair;
          },
          logger,
          idGen,
          // Setting timeout here to 150ms
          streamKeepAliveTimeoutTime: 150,
        });

        const callerInterface = await rpcClient.duplexStreamCaller<
          JSONValue,
          JSONValue
        >(methodName, { timer: 100 });

        const ctx = await ctxProm.p;
        expect(ctx.timer.delay).toEqual(100);

In the aforementioned test, the client timeout is initially set to 150ms, and then changed to 100ms, we expect the timer delay to be 100.

Issues Fixed

Related GitHub Auth Timeout is too low Polykey#588

Tasks

1. Change implementation from overriding when timeout is only lesser to always
2. Fix jests
3. Add new jests

Final checklist

ghost · 2023-10-20T04:24:16Z

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

Legend

CMCDragonkai · 2023-10-20T04:25:30Z

This needs a test for the overriding behaviour, both lesser and greater.

Also remember to squash.

CMCDragonkai · 2023-10-20T04:26:05Z

Is the best way to reset the timer vs just setting the overriding the actual timeout value?

addievo · 2023-10-20T04:33:10Z

Is the best way to reset the timer vs just setting the overriding the actual timeout value?

Idk what you mean by this

tests/RPCServer.test.ts

CMCDragonkai · 2023-10-20T04:44:06Z

Is the best way to reset the timer vs just setting the overriding the actual timeout value?

Idk what you mean by this

Can we override the timeout number?

tests/RPCServer.test.ts

CMCDragonkai · 2023-10-20T05:19:20Z

I think you need to show in a test very clearly.

Timeout at the call site overrides everything, overrides default client timeout.
Timeout for the method handler overrides default server timeout.

I don't see 1. in the test.

tests/RPCServer.test.ts

src/handlers/Handler.ts

src/RPCServer.ts

CMCDragonkai · 2023-10-20T06:07:57Z

Don't check the final checklist until the very end.

CMCDragonkai · 2023-10-20T06:08:28Z

Let's expand the scope of this PR a bit, it really should ensure that localised timeouts override global timeouts for both client and server.

CMCDragonkai · 2023-10-21T01:35:40Z

@amydevs can you take over this PR since the js-rpc actually requires consideration of the timeout architecture properly - I would want this the entire thing to be documented on the README.md the overriding order of priority.

Plus the name of the timeout parameters... is very weird, can you make them consistent between RPCServer and RPCClient.

CMCDragonkai · 2023-10-21T01:36:37Z

@amydevs see the comment here: MatrixAI/Polykey#589 (comment)

src/RPCServer.ts

tests/RPCClient.test.ts

tests/RPCServer.test.ts

addievo · 2023-10-27T00:35:11Z

Add server timeout check for positive timeouts only.
Should throw an exception if timeout is 0 or negative.

Server has a timeout to 100.
Client has timeout set to 50.
50 should take priority
When handler handles, it should take 50 as timeout.

Client timeout is 100.
Server timeout is 50.
Lesser timeout becomes the default timeout for both.

Call timeout ctx timer
This timeout will override rpcClient timeout. - This is already present.

Handler has timeout, Handler.timeout.
Handler should override the server timeout no matter the value. - This is already present.

JSON spec doesn't have Infinity.
Type of timeout property has to be null or number.
Null means Infinity in JSON.
When Parsing JSON, if timeout is null, parse it to Infinity.

tegefaulkes · 2023-10-27T01:09:15Z

Add server timeout check for positive timeouts only.

Should throw an exception if timeout is 0 or negative.

Server has a timeout to 100.

Client has timeout set to 50.

50 should take priority

When handler handles, it should take 50 as timeout.

Client timeout is 100.

Server timeout is 50.

Lesser timeout becomes the default timeout for both.

Call timeout ctx timer

This timeout will override rpcClient timeout. - This is already present.

Handler has timeout, Handler.timeout.

Handler should override the server timeout no matter the value. - This is already present.

JSON spec doesn't have Infinity.

Type of timeout property has to be null or number.

Null means Infinity in JSON.

When Parsing JSON, if timeout is null, parse it to Infinity.

This is a good start, but this information needs to be condensed into a requirement and put into the spec. We want it clear and easy to find that this is how we want the RPC to behave as a requirement.

CMCDragonkai · 2023-10-27T04:21:35Z

@addievo write up the spec in a better way onto the README.md. Use a list or table.

README.md

…est.ts`

…ault server and client timeouts regardless of their default valu fix: renamed `RPCClient.timeout` and `RPCServer.timeout` to `timeoutTIme` fix: timeout tests for RPCClient and RPCServer fix: fixed removed redundant tests [ci-skip]

chore: add tests to test negative timeout values. fix: 0 `timeoutTime` value no longer throws in `RPCClient` fix: `RPCServer.start` now throws when passed a handler with a negative `timeout` [ci-skip]

fix: `README.md` example was incorrect [ci-skip]

CMCDragonkai · 2023-10-30T21:52:02Z

Requires rebase?

tegefaulkes

Looks fine to me but it needs linting.

[ci skip]

It has been addressed.

tegefaulkes · 2023-10-31T01:08:14Z

I'm going to merge now and do a release.

addievo self-assigned this Oct 20, 2023

addievo force-pushed the feature-handler-timeout branch from ac99f3a to 1bc5ac5 Compare October 20, 2023 04:36

CMCDragonkai previously requested changes Oct 20, 2023

View reviewed changes

tests/RPCServer.test.ts Outdated Show resolved Hide resolved

tests/RPCServer.test.ts Outdated Show resolved Hide resolved

CMCDragonkai reviewed Oct 20, 2023

View reviewed changes

tests/RPCServer.test.ts Show resolved Hide resolved

addievo force-pushed the feature-handler-timeout branch from 992d374 to 61d6721 Compare October 20, 2023 05:27

CMCDragonkai reviewed Oct 20, 2023

View reviewed changes

tests/RPCServer.test.ts Outdated Show resolved Hide resolved

CMCDragonkai reviewed Oct 20, 2023

View reviewed changes

src/handlers/Handler.ts Outdated Show resolved Hide resolved

CMCDragonkai reviewed Oct 20, 2023

View reviewed changes

src/RPCServer.ts Outdated Show resolved Hide resolved

CMCDragonkai mentioned this pull request Oct 20, 2023

Encapsulate WebSocketClient in PolykeyClient MatrixAI/Polykey#582

Merged

13 tasks

CMCDragonkai assigned amydevs and unassigned addievo Oct 21, 2023

addievo assigned addievo and unassigned amydevs Oct 25, 2023

addievo changed the title ~~Feature handler timeout now overrides default timeout regardless of value~~ Update handler and caller timeouts to be able to overwrite server and client default timeouts regardless of value. Oct 25, 2023

addievo force-pushed the feature-handler-timeout branch from 80a3615 to c96c066 Compare October 25, 2023 02:19

addievo requested a review from CMCDragonkai October 25, 2023 02:19

addievo force-pushed the feature-handler-timeout branch from 60f3954 to b1c1608 Compare October 25, 2023 02:23

addievo requested a review from tegefaulkes October 25, 2023 02:34

CMCDragonkai mentioned this pull request Oct 25, 2023

Integration of js-rpc timeout and middleware changes to fix Auth Provider timeout issue MatrixAI/Polykey#589

Merged

11 tasks

tegefaulkes reviewed Oct 25, 2023

View reviewed changes

tegefaulkes mentioned this pull request Oct 25, 2023

CLI Beta Launch MatrixAI/Polykey-CLI#40

Closed

addievo force-pushed the feature-handler-timeout branch from c917ea7 to 93d535a Compare October 26, 2023 04:52

amydevs self-assigned this Oct 26, 2023

CMCDragonkai mentioned this pull request Oct 27, 2023

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

Merged

12 tasks

CMCDragonkai unassigned addievo Oct 27, 2023

amydevs force-pushed the feature-handler-timeout branch 7 times, most recently from 6f1a568 to 4e29479 Compare October 30, 2023 03:27

CMCDragonkai reviewed Oct 30, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

fix: handlerTimeoutTime renamed to timeoutTime to tests in `RPC.t…

d88cbbc

…est.ts`

amydevs force-pushed the feature-handler-timeout branch 2 times, most recently from d6d29a4 to ea8c0df Compare October 30, 2023 07:03

addievo added 2 commits October 30, 2023 18:03

feat: negative timeoutTime paramaters will now throw errors

a049c6c

chore: add tests to test negative timeout values. fix: 0 `timeoutTime` value no longer throws in `RPCClient` fix: `RPCServer.start` now throws when passed a handler with a negative `timeout` [ci-skip]

amydevs force-pushed the feature-handler-timeout branch from ea8c0df to f397972 Compare October 30, 2023 07:04

feat: Timeout Priority documentation in README.md

b835226

fix: `README.md` example was incorrect [ci-skip]

amydevs force-pushed the feature-handler-timeout branch from f397972 to b835226 Compare October 30, 2023 07:08

tegefaulkes reviewed Oct 30, 2023

View reviewed changes

lint: lint fix

9acf805

[ci skip]

tegefaulkes merged commit 928bbbc into staging Oct 31, 2023

Update handler and caller timeouts to be able to overwrite server and client default timeouts regardless of value. #47

Update handler and caller timeouts to be able to overwrite server and client default timeouts regardless of value. #47

Uh oh!

Conversation

addievo commented Oct 20, 2023 • edited by tegefaulkes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Fixed

Tasks

Final checklist

Uh oh!

ghost commented Oct 20, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Legend

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

addievo commented Oct 20, 2023

Uh oh!

Uh oh!

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

CMCDragonkai commented Oct 20, 2023

Uh oh!

CMCDragonkai commented Oct 21, 2023

Uh oh!

CMCDragonkai commented Oct 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

addievo commented Oct 27, 2023

Uh oh!

tegefaulkes commented Oct 27, 2023

Uh oh!

CMCDragonkai commented Oct 27, 2023

Uh oh!

Uh oh!

CMCDragonkai commented Oct 30, 2023

Uh oh!

tegefaulkes left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tegefaulkes commented Oct 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

addievo commented Oct 20, 2023 •

edited by tegefaulkes

Loading

ghost commented Oct 20, 2023 •

edited by ghost

Loading

tegefaulkes left a comment •

edited

Loading