Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Fix IPC handling of jsonrpc errors #1123

Merged

Conversation

oliver-giersch
Copy link
Contributor

Motivation

Fixes #1119: JSON-RPC error responses are not propagated to (request) callsites for the IPC transport correctly and end up as unhelpful serde_json errors.

Tangentially, the internal handling of all JSON values creates a lot of intermediate serde_json::Value instances, which is inefficient, since this creates potentially nested HashMaps.

Solution

Instead of calling Response::to_value, which converts both success and error responses into a serde_json::Value we call Response::into_result, which propagates error responses as Err variants.

Internal handling of JSON responses and requests is switched from serde_json::Value to serde_json::value::RawValue, which does not create any hashmaps.

Not Solved

A potential issue persists, in that internal IPC server errors (such as an unexpected disconnect of the IPC socket, e.g. a node crash) are not propagated to pending subscriptions, for other pending requests, this should be correctly propagated now, but for subscriptions this would require a change to the PubSubClient trait definition (i.e., sending Result notifications).

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great,
propagating error responses as Err variants is more useable.

probably missing cargo +nightly fmt

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@gakonst gakonst merged commit db1870c into gakonst:master Apr 9, 2022
@oliver-giersch oliver-giersch deleted the fix_handle_ipc_jsonrpc_errors branch April 15, 2022 14:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

json-rpc error responses are not handled correctly
3 participants