Factor out RPC from Polykey #8

CMCDragonkai · 2023-09-05T03:44:35Z

Description

This PR factors out the RPC functionality out of Polykey. It needs to work with https://github.com/MatrixAI/js-ws. In fact @matrixai/ws can be a devDependency in order to demonstrate the integration between WS streams and RPC. But this is not set in stone yet. We have to see what is necessary - trial by fire.

TBD - flesh out this spec by writing up all of the relevant classes and modules you're going to need. For example RPCClient, RPCServer... etc.

Issues Fixed

* Fixes RPC message parser can support arbitrary whitespace such as '\n' within and between JSON RPC messages #1
* Fixes RPC Raw Stream Errors (distinguish between transport errors and application errors) #3
* Fixes Comprehensive integration and concurrent timeout tests for agnostic RPC system #4
* Fixes Make rpc handlers abstract arrow function properties. #5
* Fixes Remove timeout grace timer for RPC system #7
* Fixes Customizable Error Filtering for js-rpc through User-Defined toError/fromError Functions #10
* Fixes IdGen function should work per call. #11

Tasks

Phase 1: Pre-Migration Assessment (Day 1)

Review RPC Classes in Polykey:

Ensure you understand how the existing classes function within Polykey.

Identify Dependencies:

Determine what dependencies the classes have within Polykey and how to replace or remove them in the standalone library.

Assess @matrixai/ws Integration:

Quickly evaluate whether @matrixai/ws can and should be integrated with the standalone library.

Phase 2: Environment Setup and Initial Migration (Day 1)

Branching and Environment Setup:

Create a new branch for the migration in the standalone library.
Install necessary dependencies, possibly including @matrixai/ws.

Initial Migration:

Copy the existing RPC classes to the new standalone library.
Make an initial commit.

Phase 3: Adaptation and Refactoring (Days 2-3)

Adapt Classes for Standalone Use:

Remove or replace any Polykey-specific dependencies or functionalities.

Refactor Code:

Make any necessary code improvements to ensure that the classes function as expected in a standalone environment.

Unit Testing:

Modify existing tests from Polykey for the standalone library or write new ones to ensure everything works as intended.

Phase 4: Integration and Advanced Testing (Day 4)

Integration with @matrixai/ws (if applicable):

Integrate the RPC functionality with @matrixai/ws. - No longer applicable

Comprehensive Tests:

Add concurrent timeout tests and other comprehensive tests.

Sanity Check:

Ensure all components are working as expected in their new environment.

Phase 5: Final Touches and PR

Documentation:

Update or add inline comments and documentation specific to the standalone library.

Linting and Code Style:

Ensure the code meets the standalone library's linting and style guidelines.

Squash and Rebase Commits:

Prepare the commit history for a clean merge.

Final Build and Testing:

Ensure everything is stable.

PR and Review:

Open a PR for the migration and address any comments that come up during the review.

Final checklist

ghost · 2023-09-05T03:45:27Z

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

Legend

CMCDragonkai · 2023-09-07T07:01:46Z

@addievo does this PR fix all the issues mentioned that this PR fixes?

Did you separate out all the handlers and other abstract classes?

Did you create the excalidraw diagram?

CMCDragonkai · 2023-09-07T07:02:24Z

Also if this is ready for review you must tick of all the tasks mentioned above. And also the final checklist too.

CMCDragonkai · 2023-09-07T07:03:12Z

Also if it fixes all of these:

Write a comment listing how each of them were solved, and which part of the code solves them. Also write a comment in each of those issues indicating how they were solved.

addievo · 2023-09-07T07:09:29Z

@addievo does this PR fix all the issues mentioned that this PR fixes?
No
Did you separate out all the handlers and other abstract classes?
Oh right, thats to be done.
Did you create the excalidraw diagram?
No

I thought the diagram wasn't part of the lib rather for pk

CMCDragonkai · 2023-09-07T07:10:20Z

No diagram for js-rpc.

WIPO Adding matrix packages to package.json WIP Changing dir WIP * Errors added new error type and sig dec for RPCServer RPCServer rewritten without PK dependency. * New utils with isObj and promise methods implemented middleware import fix WIP * Apparently fixing RPCServer.ts fixed all of import issues of RPCClient.ts as well, and client has no PK related imports now, so its fixed? #COPE WIP cping tests from pk to js-rpc experimental Decorators and changing logger version experimental Decorators and changing logger version sleep in utils.ts WIP Test resolve wip wip

addievo · 2023-09-07T07:28:09Z

Moving on to issues now, overview of issues and complexity

Fixes RPC message parser can support arbitrary whitespace such as '\n' within and between JSON RPC messages
Complexity: Moderate
Supporting arbitrary whitespace might require modifying the parser logic to handle different kinds of whitespace characters, like '\n', '\r', '\t', and spaces. JSON itself ignores whitespace, but if your RPC message format has a specific structure, you'll need to make sure that the parser can handle these variations without breaking.

Suggested Approach:
Modify the message parser to strip or appropriately handle different kinds of whitespace. Add unit tests to validate the new behavior.

Fixes RPC Raw Stream Errors (distinguish between transport errors and application errors)
Complexity: Moderate to High
Distinguishing between transport and application errors might require some architectural changes. You'll likely need to introduce an error classification system or adapt the existing one.

Suggested Approach:
Create different classes or error codes for transport and application errors and ensure that they are thrown/captured at the appropriate points in your code. Update the error handling routines to distinguish between these errors.

Fixes Make rpc handlers abstract arrow function properties
Complexity: Low to Moderate
This issue likely requires refactoring the handler classes to have abstract arrow function properties. This may involve syntactic changes and could affect type definitions.

Suggested Approach:
Refactor the handler classes to make the function properties abstract. Update any associated types or interfaces to reflect the changes.

Fixes Comprehensive integration and concurrent timeout tests for agnostic RPC system
Complexity: High
Writing comprehensive integration and concurrency tests is always a time-consuming task. It requires a deep understanding of the system, what it should and shouldn't do, and how it should behave under different conditions.

Suggested Approach:
Write individual tests for each functional part of the system, and then write integration tests that test the system as a whole. For concurrency tests, consider scenarios where multiple operations happen simultaneously and ensure the system behaves as expected.

Fixes Remove timeout grace timer for RPC system
Complexity: Moderate
This likely involves removing or modifying existing functionality related to timing out operations within the RPC system. The challenge here is to make sure that removing the grace timer doesn't introduce new issues.

Suggested Approach:
Remove the grace timer code and run comprehensive tests to ensure that the system behaves correctly without it. Make sure to consider edge cases and potential race conditions.

tests/rpc/utils/middleware.test.ts

tegefaulkes · 2023-09-13T01:18:29Z

Hey, You don't want to be resolving the issues directly. Add a "Fixes #5" line to this PR under issues fixed and it will resolve the issue when merged.

Seccondly, we want to use the conventional commits style of commit messages. For the above commit resolves issue 5, makes RPC handlers abstract arrow function propeties you want to format it more like

feat: handlers implementations are now abstract arrow functions

Some commentary here if relevant

* Related #5 

[ci skip]

Some feat could be feat, fix, build, chore, lint. There are no hard and fast rules here though. The top message starts is lowercase unless there is a noun in it, eg. fix: RPCServer something something. And no full stop.

Check out conventional commits at https://www.conventionalcommits.org/en/v1.0.0/

src/handlers/rawHandler.ts

src/callers/clientCaller.ts

src/callers/caller.ts

tsconfig.json

package.json

src/errors/errors.ts

package.json

tegefaulkes · 2023-09-13T03:48:48Z

src/utils/middleware.ts

+        const jsonMessage = messageParser(value.value);
+        const jsonMessageString = JSON.stringify(jsonMessage);
+        const jsonMessageWithNewline = jsonMessageString + '\n';
+        const jsonMessageObject: T = JSON.parse(jsonMessageWithNewline) as T;
+        controller.enqueue(jsonMessageObject);
+        bytesWritten = 0;
+      };


I don't understand what's going on here. Why are you adding a \n here? Also doing a stringify and parse within the stream here is really expensive.

Was this done to help solve #1? If so, this would not be correct way of solving this problem.

Can you answer @tegefaulkes comment above?

I don't understand what's going on here. Why are you adding a \n here? Also doing a stringify and parse within the stream here is really expensive.

Serialises the json to string, adds a new line (wherein it makes it human readable per the bare description of the issue) and then deserialises it back to an object. I now see why it might introduce a lot of overhead, lemme see how else I can resolve it. Maybe before it is sent as a stream would be a good place to change it
@tegefaulkes

Ok, I see that the issue is not clear. It does depend on a bunch of context and previous discussion. I'll expand on it in #1 in a moment.

CMCDragonkai · 2023-09-13T03:50:02Z

@addievo squashing was not done. It needs to be done first before you can tick them off.

CMCDragonkai · 2023-09-13T04:04:15Z

#4 requires tests on concurrent timeouts between client and server. Just test what happens, it should all end gracefully. If client times out before server, the server exception is just dropped.

src/index.ts

src/callers/caller.ts

src/errors/sysexits.ts

src/handlers/handler.ts

src/RPCServer.ts

addievo · 2023-09-14T00:12:23Z

ToDo:

1. Implement matrixai/events inrelation to Destroy.

Order of implementation : errors, events, RPCServer, RPCClient, jests

2. Readd error codes
3. Change errors to not not rely on sysexits, would also have to change jests.
4. Excalidraw Diagram

Previous issues, not necessary for library

5. Fixing or discarding RPC message parser can support arbitrary whitespace such as '\n' within and between JSON RPC messages #1
6. Trying to fix Remove timeout grace timer for RPC system #7

CMCDragonkai · 2023-09-14T01:36:16Z

@amydevs Can you review if the JSON serialisation/encoding makes sense for the current RPC library. Remember that when we use the RPC system in PK or PKE, we would have different application level exceptions that we would throw. Such exceptions would need to be caught and put under the cause of the root RPC error before being transmitted to the other side. When the other side receives this chained serialised error, it will try to re-instantiate it as the exception instance, but if it doesn't have one that corresponds it should either leave it as serialised form or instantiate it as an unknown error. I forgot which way we decided before - @tegefaulkes can give some pointers.

Furthermore just before the application error is serialised it should be filtered against rules on the underlying data (including all recursive cause). These rules are supposed to be provided by the application in order to prevent any leakage of sensitive data in the exception chain. This was the case before in GRPC, not sure about RPC - confirm that this is in fact tested an demonstrated that it works.

Finally with everything that has been exported make sure the index.ts is re-exporting everything fractal-like.

CMCDragonkai · 2023-09-14T01:57:48Z

Client service will have no rules and agent service will have strict rules.

amydevs · 2023-09-14T02:34:50Z

src/utils/middleware.ts

+        const jsonMessage = messageParser(value.value);
+        const jsonMessageString = JSON.stringify(jsonMessage);
+        const jsonMessageWithNewline = jsonMessageString + '\n';
+        const jsonMessageObject: T = JSON.parse(jsonMessageWithNewline) as T;


i don't think this is a good idea, JSON.parse(JSON.stringify(...)) is slow

Already had a conversation with Aditya, this solution is not correct. It should be on the parser side.

src/errors/errors.ts

src/types.ts

images/diagram_encapuslated.png

tegefaulkes

fix is a keyword in github. If you write fix #1. Fixes #1, Fixes: #1 etc etc into a commit message. It will close issue #1 when this commit merges into the main branch. Try to avoid that especially if you're not actually fixing it here.

Same thing is true for Issues and PRs.

Just something to be aware of.

CMCDragonkai · 2023-09-18T02:37:39Z

You can prefer to use WIP - ... commit messages first, then rebase squash when ready, and then when we merge, the whole PR system autocloses other issues.

src/types.ts

src/RPCClient.ts

CMCDragonkai · 2023-09-18T05:49:28Z

Notes on point 7. MatrixAI/Polykey#551 (comment) should be copied into relevant issues @addievo.

Also it wasn't written down there, but timeouts for streams should have 2 timeouts, the default timeout should be the processing of the entire entire stream. And a second optional timeout can be a per-message timeout (per-receved message in particular).

Both should default to Infinity. The second timeout is not required under unary calls. In many cases one never bothers to set the second timeout, since connection liveness is already maintained by the underlying transport whether js-ws or js-quic.

Since both timers are built into RPCClient and RPCServer, then you should have 2 relevant exceptions:

ErrorRPCClientCallerTimeout - the default timer
ErrorRPCServerHandlerTimeout - the default timer
ErrorRPCClientCallerIdleTimeout - an additional option under ctx?
ErrorRPCServerHandlerIdleTimeout - an additional optional parameter under class construction of the handler

Not sure if there's already corresponding exceptions for such things.

Important data would be the method call.

Another thing is that when timeout occurs, the reasonToCode and codeToReason should be used to signal to the other side about the stream being timed out. This again would only be done in PK not in the library. But one simply in js-rpc throws the error into the respective controllers controller.error(e).

CMCDragonkai · 2023-09-18T05:50:00Z

When @amydevs starts helping here, reserve only a couple hrs for the whitespace problem, it's not entirely critical to the testnet 6 or 7.

src/utils/utils.ts

@tegefaulkes

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]

…ng used) related: #10

# 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 assigned addievo Sep 5, 2023

addievo force-pushed the feature-rpc-factor branch 2 times, most recently from c6662b3 to 803581c Compare September 6, 2023 04:13

addievo force-pushed the feature-rpc-factor branch from 7458890 to 379e07a Compare September 7, 2023 07:14

addievo mentioned this pull request Sep 12, 2023

RPC message parser can support arbitrary whitespace such as '\n' within and between JSON RPC messages #1

Open

2 tasks

CMCDragonkai commented Sep 12, 2023

View reviewed changes

tests/rpc/utils/middleware.test.ts Outdated Show resolved Hide resolved

addievo mentioned this pull request Sep 13, 2023

Make rpc handlers abstract arrow function properties. #5

Closed