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

[addievo]

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

• 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 needs a test for the overriding behaviour, both lesser and greater.

Also remember to squash.

[CMCDragonkai]

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

[addievo]

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

Idk what you mean by this

[CMCDragonkai]

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?

[CMCDragonkai]

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

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

I don't see 1. in the test.

[CMCDragonkai]

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

[CMCDragonkai]

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]

@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]

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

[addievo]

• 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]

• 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]

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

[CMCDragonkai]

Requires rebase?

[tegefaulkes]

Looks fine to me but it needs linting.

It has been addressed.

[tegefaulkes]

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