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.

Sign up for GitHub

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

Jump to bottom

Remove timeout grace timer for RPC system #7

Closed
2 tasks
tegefaulkes opened this issue Sep 1, 2023 · 4 comments · Fixed by #8
Closed
2 tasks

Remove timeout grace timer for RPC system #7

tegefaulkes opened this issue Sep 1, 2023 · 4 comments · Fixed by #8
Assignees
@tegefaulkes
Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity

Comments

@tegefaulkes
Copy link
Contributor

Specification

In the RPC system, specifically the server side. The handler can be cancelled or timed out from the ctx: ContextTimed. This will result in a signal that SHOULD cause the handler to end immediately. It is possible for the handler to ignore this however.

Currently there is some logic what will wait a certain delay after cancellation and then force close the stream the handler was handling. This logic needs to be removed.

The reasoning is, we should allow for an arbitrary delay in handling a signal. But also, programmatic problems such as deadlocks should not be silently handled by a fail safe such as this. It should be found and dealt with during testing.

Additional context

Tasks

  1. Remove grace timeout timer logic from the RPC system.
  2. Remove any tests that test this and update ones that depend on it.
@tegefaulkes tegefaulkes added the development Standard development label Sep 1, 2023
@tegefaulkes tegefaulkes self-assigned this Sep 1, 2023
@tegefaulkes tegefaulkes changed the title remove timeout grace timer for RPC system Remove timeout grace timer for RPC system Sep 1, 2023
@CMCDragonkai
Copy link
Member

RPC issues should all go to js-rpc now. So I'm moving there.

@CMCDragonkai CMCDragonkai transferred this issue from MatrixAI/Polykey Sep 5, 2023
@CMCDragonkai CMCDragonkai mentioned this issue Sep 5, 2023
Merged
31 tasks
@addievo addievo pinned this issue Sep 13, 2023
@addievo
Copy link
Contributor

addievo commented Sep 18, 2023 •
edited
Loading

  1. The AbortController and Timer logic seems to be implemented for each handler and is used to abort the process or refresh the timeout. The grace timeout mechanism looks to be enforced through a separate Timer (graceTimer).
  2. To remove this grace timeout, remove or comment out the code related with the graceTimer and its event listener (handleAbort).
  3. Remove jests related to same.
  • Remove graceTimer
  • Remove event listener
  • Remove jests

addievo added a commit that referenced this issue Sep 18, 2023
@addievo
fix: fixes #7
6f11b4e 
* Removes graceTimer and related jests
@CMCDragonkai
Copy link
Member

Yes grace timer removal is because of these reasons:

  1. Timeouts are advisory always. This is because JS is a cooperative concurrency system not a pre-emptive concurrency system. This advisory cancellation is fundamental.
  2. There is an option for the decorator called lazy. If it is false, the promise rejects but that doesn't mean the underlying operation is finished. It's just an early rejection.
  3. In all cases where IO is involved, early rejection is incorrect. Therefore all uses of times cancellability of RPC calls must have lazy set to true.
  4. We can enforce this in the RPC handlers and callers or... Which would be easier than expecting the user to always set it up correctly. However I'm not sure about ergonomics. Provide some examples of tradeoffs here @addievo

@CMCDragonkai
Copy link
Member

Also remember to tick off the tasks in the OP and not just create new tasks. Always update the OP with tasks.

addievo added a commit that referenced this issue Sep 26, 2023
@addievo
Works as a standalone library now. Pending review to merge to staging.
f30da25 
callers and handlers are now refactored

* WIP - Newline now works, refers issue #1

node v20 fix

feat: handlers implementations are now abstract arrow functions

* Fixes #5

[ci skip]

* resolves issue 5, makes RPC handlers abstract arrow function properties

feat: rename to uppercase

[ci skip]

fix: handler export fix

[ci skip]

fix: tsconf from quic

[ci skip]

fix: dependencies (js quic), events and errors versions, changing to relative imports, jest dev dependency, js-quic tsconfig

[ci skip]

fix: tests imports, using @

[ci skip]

chore: removed sysexits

chore: fix default exports for callers and handlers
Fixed index for handlers

fix: remove @matrixai/id

fix: remove @matrixai/id and ix

chore : diagram

[ci skip]

chore : lintfix
fix: errors now extend AbstractError

[ci skip]

fix: undoing fix #1

[ci skip]

replacd errorCode with just code, references std error codes from rpc spec

feat: events based createDestroy

[ci skip]

chore: img format fix

[ci skip]

chore: img in README.md

[ci skip]

feat: allows the user to pass in a generator function if the user wishes to specify a particular id

[ci skip]

fix: fixes #7

* Removes graceTimer and related jests

chore: idGen name change. idGen parameter in creation and constructor. No longer optional. Only defaulted in one place.

wip: added idgen to jests, was missing.

[ci skip]

wip: reimported ix, since a few tests rely on it.
removed, matrixai/id

wip: jests for #4
removed, matrixai/id

wip: * Implements custom RPC Error codes.
     * Fixed jest for concurrent timeouts
     * All errors now have a cause
     * All errors now use custom error codes.

wip: *Client uses ctx timer now

wip: *Jests to test concurrency

wip: *custom RPC based errors for RPC Client, now all errors have a cause and an error message

WIP: * Refactor out sensitiveReplacer

WIP: * Refactor out sensitiveReplacer

WIP: * Update to latest async init and events
* set default timeout to Infinity
* jest to check server and client with infinite timeout
* fixing jests which broke after changing default timeout to infinity

WIP: f1x #4

WIP: f1x #11

f1x: parameterize toError, fromError and replacer

wip: tofrom

fix: parameterize toError, fromError and replacer

fix: Makes concurrent jests non deterministic

* Related #4

fix: parameterize replacer toError and fromError, change fromError to return JSONValue, stringify fromError usages

* Related #10

fix: Converted global state for fromError to handle it internally.

*Related: #10
Reviewed-by: @tegefaulkes
[ci skip]

chore: Jests for fromError and toError, and using a custom replacer.

related: #10

[ci skip]
addievo pushed a commit that referenced this issue Oct 9, 2023
@addievo
# This is a combination of 16 commits.
63e13af 
# This is the 1st commit message:

fix: RPCServer.start is now no longer a factory function

fix: fixed RPC tests after async-init change

# This is the commit message #2:

chore: updated inline documentation according to async-init changes

# This is the commit message #3:

wip: fixing imports

# This is the commit message #4:

wip: RPCServer deconstruction

# This is the commit message #5:

fix: RPCServer start stop order

# This is the commit message #6:

fix: added back createDestroy back to RPCClient

# This is the commit message #7:

chore: lintfix

# This is the commit message #8:

wip: completely removed create destroy from rpcclient

# This is the commit message #9:

fix: RPCClient tests after async-init changes

# This is the commit message #10:

wip

# This is the commit message #11:

fix: idgen is optional in constructor

# This is the commit message #12:

fix: type import in ./types

# This is the commit message #13:

chore: test for RPCServer force stopping

# This is the commit message #14:

fix: proper implementation of fromError toError, clientOutputTransformStream no longer outputs error.data, but rather error directly

# This is the commit message #15:

fix: jest fix for ErrorRPCRemote

# This is the commit message #16:

wip toError fromError

fix: proper implementation of fromError toError, clientOutputTransformStream no longer outputs error.data, but rather error directly

fix: changed rpcClient toError implementation

fix: changing ErrorRPCRemote constructor to be in-line with toError error creation constructor

fix: jest fix for ErrorRPCRemote

fix: fixing exports

fix: RPC errors now correctly extend AbstractError

fix: removed old events
fix: cleaned up imports
feat: client has toError as parameter
fix: removed type from JSONError obj
fix: startStop constructor correctly used
fix: infinity is definitely not == 1 minute

chore:  rename variable
@CMCDragonkai CMCDragonkai added r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tegefaulkes tegefaulkes

Labels
development Standard development r&d:polykey:core activity 1 Secret Vault Sharing and Secret History Management r&d:polykey:supporting activity Supporting core activity
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Factor out RPC from Polykey MatrixAI/js-rpc
3 participants
@CMCDragonkai @tegefaulkes @addievo