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

Remove AddChainOptions.jsonRpcCallback in favor of Chain.nextJsonRpcResponse #2778

Merged
merged 18 commits into from
Sep 28, 2022

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Sep 25, 2022

Close #2528

This PR is a major change in the public API of the JS library, as such I will bump to 0.7.0 at the next release.

Instead of passing a callback when adding a chain where the JSON-RPC responses are sent, you are now supposed to call nextJsonRpcResponse on the chain.
This asynchronous function waits (if necessary) for a JSON-RPC response to be available then returns it.

In addition to this, sendJsonRpc now returns an error if the request is malformed or if the queue is full.

This change in API has several major advantages:

  • It removes an ambiguity about what to do if the jsonRpcCallback throws an exception.
  • It is now possible to back-pressure the client by not calling nextJsonRpcResponse. This will cause the responses to pile up in the buffer, until a point where sendJsonRpc will return an error.
  • It is overall easier to use the API because you no longer have to create things ahead of time (before the chain is even created) in order to pass them to the callback. See the tests that this PR rewrites for example.
  • Smoldot crashes are now reported faster, because nextJsonRpcResponse will throw an exception in case of a smoldot crash, while before the callback was simply not called and you had to detect crashes through sendJsonRpc.

Migration

It is always possible to migrate to the new API.

If your code looks like this:

const chain = await client.addChain({ jsonRpcCallback, /* other options */ ... });

You can rewrite it like this:

const chain = await client.addChain({ /* other options */ ... });

(async () => {
    try {
        while(true) {
            const response = await chain.nextJsonRpcResponse();
            /* do whatever you want here */
        }
    } catch(_error) {
        // Will happen when the chain is removed or the client terminated.
    }
})()

Copy link
Contributor

@mergify mergify bot left a comment

Choose a reason for hiding this comment

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

Automatically approving tomaka's pull requests. This auto-approval will be removed once more maintainers are active.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2022

twiggy diff report

Difference in .wasm size before and after this pull request.


 Delta Bytes │ Item
─────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────
        +567 ┊ json_rpc_responses_peek
        -329 ┊ <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll::h4e492bf7c95251a4
        +325 ┊ slab::Slab<T>::try_remove::hafd44190d6748626
        +323 ┊ slab::Slab<T>::insert_at::h3ffd863ab50737b8
        -273 ┊ smoldot_light::json_rpc_service::HandleRpcError::into_json_rpc_error::h668ff6c8e4e9d02f
        -259 ┊ slab::Slab<T>::insert_at::h9b8c4d3e6b590f03
        -237 ┊ slab::Slab<T>::try_remove::hbae251b27e493421
        +230 ┊ alloc::raw_vec::RawVec<T,A>::grow_amortized::h000ca4485f7728db
        -230 ┊ alloc::raw_vec::RawVec<T,A>::grow_amortized::h0a932a824e8e7b30
        -210 ┊ std::sync::once::Once::call_once::{{closure}}::h5c628c06e1a901e3
        +210 ┊ std::sync::once::Once::call_once::{{closure}}::hd524503089a049e8
        +207 ┊ json_rpc_responses_pop
        -181 ┊ alloc::raw_vec::RawVec<T,A>::allocate_in::h90eb24fcf9b458af
        -181 ┊ alloc::raw_vec::RawVec<T,A>::allocate_in::habbb60f6cd4f5642
        +181 ┊ alloc::raw_vec::RawVec<T,A>::allocate_in::hc5c225bb9e8dd353
        -147 ┊ <alloc::vec::Vec<T,A> as alloc::vec::spec_extend::SpecExtend<T,I>>::spec_extend::h012330e36810ebab
        -139 ┊ <futures_util::future::future::map::Map<Fut,F> as core::future::future::Future>::poll::ha3a549da3617399f
        +135 ┊ slab::Slab<T>::remove::hc11d7b8e4935c716
        +117 ┊ remove_chain
        +113 ┊ slab::Slab<T>::insert::hebb2c39d01a3ac74
        +130 ┊ ... and 66 more.
        -332 ┊ Σ [86 Total Rows]

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@tomaka tomaka added the automerge Automatically merge pull request as soon as possible label Sep 28, 2022
@mergify mergify bot merged commit 8bb31f9 into paritytech:main Sep 28, 2022
@tomaka tomaka deleted the different-responses-system branch September 28, 2022 10:00
mergify bot added a commit that referenced this pull request Sep 28, 2022
Close #2456

This PR removes the `Chain.databaseContent` function from the public
API. Users are supposed to use the
`chainHead_unstable_finalizedDatabase` JSON-RPC function that was added
in #2749 instead.

This is an API breaking change. Since
#2778 has been merged and is a
breaking change, let's do other breaking changes at the same time.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge pull request as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace JSON-RPC callback with a Promise-based API
2 participants