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

IdGen function should work per call. #11

Closed
1 task done
addievo opened this issue Sep 21, 2023 · 4 comments · Fixed by #8
Closed
1 task done

IdGen function should work per call. #11

addievo opened this issue Sep 21, 2023 · 4 comments · Fixed by #8
Assignees
@addievo
Labels
development Standard development r&d:polykey:supporting activity Supporting core activity

Comments

@addievo
Copy link
Contributor

addievo commented Sep 21, 2023 •
edited by CMCDragonkai
Loading

Refactor IdGen to assign IDs on a per-call basis for streamed RPC responses, ensuring that all responses in a stream share the same ID as the originating request.

Description

In the existing setup, IdGen assigns unique IDs to each individual RPC request. This becomes problematic when streaming multiple responses, as each response should have an ID identical to that of the original request, according to the JSON RPC Spec.

Tasks

  • 1. Refactor IdGen so that it applies ID on a per-call basis.
@addievo addievo added the development Standard development label Sep 21, 2023
@CMCDragonkai
Copy link
Member

Draw up the unary, client streaming, server streaming, duplex streaming examples as I had on the whiteboard.

@CMCDragonkai
Copy link
Member

I had a thought that ID gen could also generate subids for each serial message in a call. But that would be something for the future.

@addievo
Copy link
Contributor Author

addievo commented Sep 21, 2023

idgen stuff

@addievo addievo mentioned this issue Sep 21, 2023
Merged
31 tasks
addievo added a commit that referenced this issue Sep 21, 2023
@addievo
WIP: f1x #11
330cac3
@tegefaulkes
Copy link
Contributor

If you want to avoid using fix in the commit message I suggest formatting it like this.

fix: fixes problem with id generation

* Related: #11

[ci skip]

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 the r&d:polykey:supporting activity Supporting core activity label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@addievo addievo

Labels
development Standard development 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