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

polykey identities authenticate Timeout Upped to 2 Minutes #49

Closed
wants to merge 1 commit into from

Conversation

amydevs
Copy link
Member

@amydevs amydevs commented Nov 1, 2023

Description

The polykey identities authenticate command currently follows the default timeout of RPCClient (15 seconds). Since RPC changes have been made so that the call-site ctx can override the global timeout, it can be upped to 2 minutes without affecting the timeouts of other handlers.

Issues Related

Tasks

  • 1. Bump Polykey version
  • 2. Add { timer: 120000 } to ctx of polykey identities authenticate to extend timeout

Final checklist

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

@ghost
Copy link

ghost commented Nov 1, 2023

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

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@CMCDragonkai CMCDragonkai mentioned this pull request Nov 5, 2023
@amydevs
Copy link
Member Author

amydevs commented Nov 6, 2023

blocked on PK CI passing

@CMCDragonkai
Copy link
Member

Just link it for now until @tegefaulkes fixes it up. And then prepare it for merging? Unless you're saying it works?

@tegefaulkes
Copy link
Contributor

I looked into the problem in polykey with the CI failing. Seems to be a problem with the PR that recently merged, not with my changes.

I'm getting the following error pretty consistently.

 console.error
    TypeError: Cannot create property 'metadata' on string 'hello'
        at Object.transform (/home/faulkes/matrixcode/polykey/Polykey/node_modules/@matrixai/rpc/src/middleware.ts:132:67)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at transformStreamDefaultControllerPerformTransform (node:internal/webstreams/transformstream:509:18)
        at transformStreamDefaultSinkWriteAlgorithm (node:internal/webstreams/transformstream:559:10)
        at Object.write (node:internal/webstreams/transformstream:364:14)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1114:5)
        at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1229:5)
        at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1103:3)
        at writableStreamDefaultWriterWrite (node:internal/webstreams/writablestream:993:3)
        at Object.[kChunk] (node:internal/webstreams/readablestream:1404:28)
        at readableStreamFulfillReadRequest (node:internal/webstreams/readablestream:1996:24)
        at readableStreamDefaultControllerEnqueue (node:internal/webstreams/readablestream:2187:5)
        at transformStreamDefaultControllerEnqueue (node:internal/webstreams/transformstream:490:5)
        at defaultTransformAlgorithm (node:internal/webstreams/transformstream:349:3)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at transformStreamDefaultControllerPerformTransform (node:internal/webstreams/transformstream:509:18)
        at transformStreamDefaultSinkWriteAlgorithm (node:internal/webstreams/transformstream:559:10)
        at Object.write (node:internal/webstreams/transformstream:364:14)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1114:5)
        at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1229:5)
        at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1103:3)
        at writableStreamDefaultWriterWrite (node:internal/webstreams/writablestream:993:3)
        at Object.[kChunk] (node:internal/webstreams/readablestream:1404:28)
        at readableStreamFulfillReadRequest (node:internal/webstreams/readablestream:1996:24)
        at readableStreamDefaultControllerEnqueue (node:internal/webstreams/readablestream:2187:5)
        at transformStreamDefaultControllerEnqueue (node:internal/webstreams/transformstream:490:5)
        at TransformStreamDefaultController.enqueue (node:internal/webstreams/transformstream:301:5)
        at Object.transform (/home/faulkes/matrixcode/polykey/Polykey/node_modules/@matrixai/rpc/src/utils.ts:427:18)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at transformStreamDefaultControllerPerformTransform (node:internal/webstreams/transformstream:509:18)
        at transformStreamDefaultSinkWriteAlgorithm (node:internal/webstreams/transformstream:559:10)
        at Object.write (node:internal/webstreams/transformstream:364:14)
        at ensureIsPromise (node:internal/webstreams/util:182:19)
        at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1114:5)
        at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1229:5)
        at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1103:3)
        at writableStreamDefaultWriterWrite (node:internal/webstreams/writablestream:993:3)
        at WritableStreamDefaultWriter.write (node:internal/webstreams/writablestream:499:12)
        at RPCClient.unaryCaller (/home/faulkes/matrixcode/polykey/Polykey/node_modules/@matrixai/rpc/src/RPCClient.ts:140:20)
        at /home/faulkes/matrixcode/polykey/Polykey/tests/nodes/NodeConnectionManager.lifecycle.test.ts:269:9
        at /home/faulkes/matrixcode/polykey/Polykey/src/nodes/NodeConnectionManager.ts:703:16
        at withF (/home/faulkes/matrixcode/polykey/Polykey/node_modules/@matrixai/resources/src/utils.ts:24:12)
        at constructor_.withConnF (/home/faulkes/matrixcode/polykey/Polykey/src/nodes/NodeConnectionManager.ts:700:12)

      329 |  */
      330 | const reasonToCode = (_type: 'read' | 'write', reason?: any): number => {
    > 331 |   console.error(reason)
          |           ^
      332 |   // We're only going to handle RPC errors for now, these include
      333 |   // ErrorRPCHandlerFailed
      334 |   // ErrorRPCMessageLength

      at constructor_.error [as reasonToCode] (src/nodes/utils.ts:331:11)
      at constructor_.writableAbort (node_modules/@matrixai/quic/src/QUICStream.ts:839:23)

I don't think all the polykey tests were updated to reflect the changes in js-rpc.

@amydevs
Copy link
Member Author

amydevs commented Nov 6, 2023

This is blocked by #53 due to introduction of MDNS in polykey alpha-20

@amydevs
Copy link
Member Author

amydevs commented Nov 9, 2023

image

This is working as intended.

However, it is important to note, that it is not actually the RPC timing out, but the method on GithubProvider that is timing out.

This is because of changes implemented in MatrixAI/js-rpc#53. Because the server streaming call sends the client a message at the start for the Authentication code, the timeout is cancelled on both the server and the client.

This should be fine for our usecase right now. BUT it will mean that we can't have the client dictate a shorter timeout. There are a few ways to solve this:

  • Use a ctx in Provider.Authenticate, so that when the server middleware refreshes with a shorter timeout conveyed by the client, the method will use that to update the timeout accordingly
  • If you pass a Timer instance to a call-site, it is not cancelled when a message is received from the server. So this Timer can be used for cancelling the server streaming call.
  • Implement a way to escape the current timeout behaviour and use the old timeout behaviour (timeout per message).

btw, this PR in the current state, will not change anything. As the changes in Polykey (upping the timeout) and js-rpc (making timeout timers be cancelled after the first server-sent message) have made it so that this PR is not needed UNLESS the changes I've suggested above are going to be implemented

@CMCDragonkai
Copy link
Member

Use a ctx in Provider.Authenticate, so that when the server middleware refreshes with a shorter timeout conveyed by the client, the method will use that to update the timeout accordingly

The authenticate needs to be refactored to use the ctx now. It should be a cancellable decorator.

The lifetime of the authentication will now depend on the lifetime of the SS call. If the client then closes the stream, or the stream is closed for some other reason, then you want to abort the authentication process.

@CMCDragonkai
Copy link
Member

So the server side handler has already defaulted to 120000 so this PR is no longer required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants