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

RPC Raw Stream Errors (distinguish between transport errors and application errors) #3

Closed
tegefaulkes opened this issue Jul 27, 2023 · 4 comments · Fixed by #8
Closed
Assignees
Labels
development Standard development

Comments

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Jul 27, 2023

Specification

We need to review how errors are handled for RPC raw handlers. The transport level error handling needs to be reviewed as well. Error handling between websocket streams and QUIC streams need to be stadardized between each other as well.

There needs to be separation between transport level errors and RPC level ones. I think this is already the case.

Additional context

Tasks

  1. ...
  2. ...
  3. ...
@CMCDragonkai
Copy link
Member

When we drop to raw streams, we cannot make use of JSON RPC to transmit errors anymore. If an error were to occur, it has to be done in whatever the raw stream is doing, which depends on the handler managing the raw stream. It could be using CBOR or protobuf... But in some cases, the raw stream is just raw binary data. Say for example uploading a file from Polykey CLI to Polykey Agent. In that situation, if there's an error, it's only possible to send it at the transport level.

Transport level error capability can be limited. In some cases only a numeric code can be transmitted. We should allow for some ability of choosing these codes. A message could be supplied, however if the transport layer doesn't support it, then it just ignores the message.

A generic exception for transport layer errors should then be raised to the other side. It should be possible to distinguish 3 kinds of errors in the transport layer:

  1. System level errors
  2. Application errors
  3. Application due to RPC raw stream errors

Usually 1 and 2 are distinguishable by some sort of code. However 2 and 3 may need an additional code, or a subcode or something.

@CMCDragonkai CMCDragonkai changed the title Review RPC and transport error handling. RPC Raw Stream Errors (distinguish between transport errors and application errors) Aug 1, 2023
@CMCDragonkai
Copy link
Member

Moved to js-rpc as it would be done there.

@CMCDragonkai CMCDragonkai transferred this issue from MatrixAI/Polykey Aug 11, 2023
@CMCDragonkai
Copy link
Member

This can only be done after MatrixAI/js-ws#1 and MatrixAI/js-ws#2 is done first. Because in order to separate 3 levels of errors:

  • Connection Errors
  • Stream Errors
  • RPC Call Errors

The concept of a "stream" needs to exist above a "WebSocket" connection.

For js-quic this is already done due to QUICStream.

@addievo
Copy link
Contributor

addievo commented Sep 20, 2023

  • Re-introduce codes which was refactored out and replace them with RPC specific codes instead of PK codes.
  • Write specific errors for RPC -> -32000 - -32099
  • Implement codes in src.

(Cope concurrent jests get fixed.)

  • Implement with toError and fromError.
  • Jests?

@addievo addievo closed this as completed Sep 26, 2023
addievo pushed a commit that referenced this issue Oct 9, 2023
# 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development
Development

Successfully merging a pull request may close this issue.

3 participants