server: Register raw method with connection ID #1297

lexnv · 2024-02-21T15:25:31Z

This PR exposes the connection ID to raw methods.

To achieve this, a new register_raw_method is exposed on the RpcModule.
The with-context attribute is implemented in the proc-macros to use this method.
When the with-context attribute is used, all methods of a server are rendered with an extra connection Id parameter.
This is similar to the PendingSubscriptionSink that is injected into subscription methods.

Examples

The example server_with_context implements a server with the context attribute enabled.
A function of the server returns the connection ID from its parameters.
Users on different connections will receive a different connection ID.

Testing

Add proc-macro tests to capture the incompatibilities between with-context, sync methods and blocking methods
Add a unit-test to validate this behavior further
Build on the example to allow methods to be called from a single connection context

A simplified version of #1295, that avoids a user-breaking change.

Signed-off-by: Alexandru Vasile <[email protected]>

core/src/server/rpc_module.rs

examples/examples/server_with_context.rs

niklasad1

This should work and overall I like it but some small comments to address

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1

LGTM, nice work

I wonder if we should provide a type with connection_id() to avoid breaking it if we decide to add some other field but I think that's unlikely though.

Signed-off-by: Alexandru Vasile <[email protected]>

tests/tests/helpers.rs

Signed-off-by: Alexandru Vasile <[email protected]>

jsdw

Looks good to me; just a couple of thoughts:

So, basically, register_raw_method is the same as register_async_method but with a connection ID. I'd wonder about calling it something more similar to that, eg: register_async_method_with_details (especially if the below was done).

In general I guess being able to pass Extensions between middlewares and method would remove the need for this again? But happy to just merge this and cross that bridge when we get there anyways!

I wonder if we should provide a type with connection_id() to avoid breaking it if we decide to add some other field but I think that's unlikely though.

I wondered too about wrapping the ConnectionId in a type, just to give more flexibility eg:

pub struct ConnectionDetails {
    id: ConnectionId
}

impl ConnectionDetails {
    pub fn id(&self) -> &ConnectionId
}

…text-api-v2 Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

core/src/server/rpc_module.rs

niklasad1 · 2024-03-04T16:02:19Z

core/src/server/rpc_module.rs

+
+#[derive(Debug, Clone)]
+#[allow(missing_copy_implementations)]
+/// The connection details exposed to the server methods.


I don't understand why we need the builder?

The RpcService should be the last "layer" before the user-callback is called.

We construct the ConnectionDetails in this layer:

jsonrpsee/server/src/middleware/rpc/layer/rpc_service.rs

Lines 102 to 104 in 87585ad

let connection_details = ConnectionDetails::builder().id(conn_id).build();

let fut = (callback)(id, params, connection_details, max_response_body_size);

ResponseFuture::future(fut)

I think we could just as easily provide a constructor for this:

impl ConnectionDetails { pub fn new(id: ConnectionId) .. }

The new method needs to be public, because the ConnectionDetails is declared in the core crate. And the ConnectionDetails needs to be instantiated in the server crate as well.

We'd have to eventually change this method to new(id: ConnectionId, extensions: Extenstions) which will be a breaking change (although we should be the only ones building this object).

TLDR; since we'll do a breaking change either way, I think a new method should be fine for now! 🙏

ok, I see makes sense

Maybe have it be #[doc(hidden)] and comment that it's not part of the public APi and can change without notice? I don't think anything outside of jsonrpsee needs to build it, so then we could also just use new() instead of a builder (Or maybe eg _new(connection_id) to really emphasise the hiddenness of it but shrug)

niklasad1 · 2024-03-04T16:06:41Z

core/src/server/rpc_module.rs

+	/// module.register_async_with_details("say_hello", |_params, _connection_details, _ctx| async { "lo" }).unwrap();
 	/// ```
-	pub fn register_raw_method<R, Fun, Fut>(
+	pub fn register_async_with_details<R, Fun, Fut>(


I'm not super happy about the naming register_async_with_details, it's too vague for my taste.
I would prefer register_async_method_with_conn_context or something like that

It should be obvious whether it is a method call or subscription IMO.

How about register_async_method_eith_details? since we are provigin ConnectionDetails to it, so the word details lines up well imo :)

Co-authored-by: Niklas Adolfsson <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

…ethod_with_details` Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

…into lexnv/low-level-context-api-v2

niklasad1 · 2024-03-05T08:52:22Z

We may mark this entire feature/API as #[doc(hidden)] as we will break it anyway in the next release?

What do you reckon?

jsdw · 2024-03-05T09:24:18Z

We may mark this entire feature/API as #[doc(hidden)] as we will break anyway in the next release?

Works for me too, since I geuss we only need it internally right now and we'll break it again soon! Could also be feature flagged to hdie it at compile time and then only enable in substrate?

Signed-off-by: Alexandru Vasile <[email protected]>

core/src/server/rpc_module.rs

niklasad1 · 2024-03-05T13:28:40Z

examples/examples/server_with_raw_method.rs

rename perhaps?

Have changed it to server_wtih_connection_details, thanks!

Signed-off-by: Alexandru Vasile <[email protected]>

Co-authored-by: Niklas Adolfsson <[email protected]>

…into lexnv/low-level-context-api-v2 Signed-off-by: Alexandru Vasile <[email protected]>

Signed-off-by: Alexandru Vasile <[email protected]>

core/src/server/rpc_module.rs

… context and limit connections (#3481) This PR ensures that the chainHead RPC class can be called only from within the same connection context. The chainHead methods are now registered as raw methods. - paritytech/jsonrpsee#1297 The concept of raw methods is introduced in jsonrpsee, which is an async method that exposes the connection ID: The raw method doesn't have the concept of a blocking method. Previously blocking methods are now spawning a blocking task to handle their blocking (ie DB) access. We spawn the same number of tasks as before, however we do that explicitly. Another approach would be implementing a RPC middleware that captures and decodes the method parameters: - #3343 However, that approach is prone to errors since the methods are hardcoded by name. Performace is affected by the double deserialization that needs to happen to extract the subscription ID we'd like to limit. Once from the middleware, and once from the methods itself. This PR paves the way to implement the chainHead connection limiter: - #1505 Registering tokens (subscription ID / operation ID) on the `RpcConnections` could be extended to return an error when the maximum number of operations is reached. While at it, have added an integration-test to ensure that chainHead methods can be called from within the same connection context. Before this is merged, a new JsonRPC release should be made to expose the `raw-methods`: - [x] Use jsonrpsee from crates io (blocked by: paritytech/jsonrpsee#1297) Closes: #3207 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>

… context and limit connections (paritytech#3481) This PR ensures that the chainHead RPC class can be called only from within the same connection context. The chainHead methods are now registered as raw methods. - paritytech/jsonrpsee#1297 The concept of raw methods is introduced in jsonrpsee, which is an async method that exposes the connection ID: The raw method doesn't have the concept of a blocking method. Previously blocking methods are now spawning a blocking task to handle their blocking (ie DB) access. We spawn the same number of tasks as before, however we do that explicitly. Another approach would be implementing a RPC middleware that captures and decodes the method parameters: - paritytech#3343 However, that approach is prone to errors since the methods are hardcoded by name. Performace is affected by the double deserialization that needs to happen to extract the subscription ID we'd like to limit. Once from the middleware, and once from the methods itself. This PR paves the way to implement the chainHead connection limiter: - paritytech#1505 Registering tokens (subscription ID / operation ID) on the `RpcConnections` could be extended to return an error when the maximum number of operations is reached. While at it, have added an integration-test to ensure that chainHead methods can be called from within the same connection context. Before this is merged, a new JsonRPC release should be made to expose the `raw-methods`: - [x] Use jsonrpsee from crates io (blocked by: paritytech/jsonrpsee#1297) Closes: paritytech#3207 cc @paritytech/subxt-team --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Niklas Adolfsson <[email protected]>

lexnv added 5 commits February 21, 2024 14:54

server: Add a raw method

26ec635

Signed-off-by: Alexandru Vasile <[email protected]>

server: Register raw methods, blocking or unblocking

ef24148

Signed-off-by: Alexandru Vasile <[email protected]>

proc-macros: Add with-context attribute

4f8eb03

Signed-off-by: Alexandru Vasile <[email protected]>

server: Register sync and nonblocking methods for raw API

9eba899

Signed-off-by: Alexandru Vasile <[email protected]>

examples: Add with context example

239cafe

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv requested a review from a team as a code owner February 21, 2024 15:25

lexnv marked this pull request as draft February 21, 2024 15:25

lexnv added 2 commits February 21, 2024 17:55

core: Adjust docs for the raw method registering

0de6d95

Signed-off-by: Alexandru Vasile <[email protected]>

proc-macros: Cargo fmt

4ce51c6

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 reviewed Feb 21, 2024

View reviewed changes

core/src/server/rpc_module.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Feb 21, 2024

View reviewed changes

examples/examples/server_with_context.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Feb 21, 2024

View reviewed changes

lexnv added 9 commits February 22, 2024 17:20

server: Request Arc<Context> for the raw method callback

5f2b40a

Signed-off-by: Alexandru Vasile <[email protected]>

proc-macros: Per method raw-method attribute

753224a

Signed-off-by: Alexandru Vasile <[email protected]>

examples: Add server raw method

d526b70

Signed-off-by: Alexandru Vasile <[email protected]>

tests/ui: Check correct proc-macro behavior

c8ff969

Signed-off-by: Alexandru Vasile <[email protected]>

tests/ui: Negative test for async with raw methods

b4880b9

Signed-off-by: Alexandru Vasile <[email protected]>

tests/ui: Negative test for blocking with raw methods

d2da4b0

Signed-off-by: Alexandru Vasile <[email protected]>

tests/proc-macros: Ensure unique connection IDs from different clients

0dc48e9

Signed-off-by: Alexandru Vasile <[email protected]>

tests/integration: Ensure unique connection IDs from different clients

0029418

Signed-off-by: Alexandru Vasile <[email protected]>

proc-macros: Apply cargo fmt

3a09a7e

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 approved these changes Feb 22, 2024

View reviewed changes

lexnv marked this pull request as ready for review February 22, 2024 17:16

Register raw method as async method

1565927

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 reviewed Feb 23, 2024

View reviewed changes

tests/tests/helpers.rs Outdated Show resolved Hide resolved

lexnv added 2 commits February 23, 2024 15:34

Fix testing

f5202ef

Signed-off-by: Alexandru Vasile <[email protected]>

core: Fix documentation

8cf1947

Signed-off-by: Alexandru Vasile <[email protected]>

lexnv mentioned this pull request Feb 26, 2024

chainHead: Allow methods to be called from within a single connection context and limit connections paritytech/polkadot-sdk#3481

Merged

1 task

niklasad1 mentioned this pull request Mar 1, 2024

server: Extend methods with optional connection context #1295

Closed

jsdw approved these changes Mar 4, 2024

View reviewed changes

lexnv added 5 commits March 4, 2024 16:29

Merge remote-tracking branch 'origin/master' into lexnv/low-level-con…

bd571e1

…text-api-v2 Signed-off-by: Alexandru Vasile <[email protected]>

server: Rename raw method to module.register_async_with_details

04dfd25

Signed-off-by: Alexandru Vasile <[email protected]>

server: Add connection details wrapper

e02ba61

Signed-off-by: Alexandru Vasile <[email protected]>

server: Add asyncWithDetails and connection details

7b72d8a

Signed-off-by: Alexandru Vasile <[email protected]>

proc-macros: Provide connection details to methods

87585ad

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 reviewed Mar 4, 2024

View reviewed changes

core/src/server/rpc_module.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Mar 4, 2024

View reviewed changes

lexnv and others added 5 commits March 4, 2024 18:41

Update core/src/server/rpc_module.rs

2c50375

Co-authored-by: Niklas Adolfsson <[email protected]>

server: Remove connection details builder

66b6a39

Signed-off-by: Alexandru Vasile <[email protected]>

server: Refactor .register_async_with_details to `.register_async_m…

cfe1aec

…ethod_with_details` Signed-off-by: Alexandru Vasile <[email protected]>

proc-macro: Clarify comment

7327d8b

Signed-off-by: Alexandru Vasile <[email protected]>

Merge remote-tracking branch 'origin/lexnv/low-level-context-api-v2' …

6605409

…into lexnv/low-level-context-api-v2

core: Doc hidden for async with details

650e24a

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 reviewed Mar 5, 2024

View reviewed changes

core/src/server/rpc_module.rs Show resolved Hide resolved

niklasad1 reviewed Mar 5, 2024

View reviewed changes

lexnv and others added 4 commits March 5, 2024 15:35

Rename example

f4d9085

Signed-off-by: Alexandru Vasile <[email protected]>

Update core/src/server/rpc_module.rs

40d2bcf

Co-authored-by: Niklas Adolfsson <[email protected]>

Merge remote-tracking branch 'origin/lexnv/low-level-context-api-v2' …

8185193

…into lexnv/low-level-context-api-v2 Signed-off-by: Alexandru Vasile <[email protected]>

core: Remove doc(hidden) from ConnectionDetails::id

11ef18c

Signed-off-by: Alexandru Vasile <[email protected]>

niklasad1 reviewed Mar 5, 2024

View reviewed changes

core/src/server/rpc_module.rs Outdated Show resolved Hide resolved

Update core/src/server/rpc_module.rs

4f459ac

niklasad1 merged commit d3b1b3a into master Mar 5, 2024

niklasad1 deleted the lexnv/low-level-context-api-v2 branch March 5, 2024 15:03

lexnv mentioned this pull request Mar 5, 2024

chore: Release 0.22.2 #1313

Merged

	let connection_details = ConnectionDetails::builder().id(conn_id).build();
	let fut = (callback)(id, params, connection_details, max_response_body_size);
	ResponseFuture::future(fut)

server: Register raw method with connection ID #1297

server: Register raw method with connection ID #1297

Uh oh!

Conversation

lexnv commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Examples

Testing

Uh oh!

Uh oh!

Uh oh!

niklasad1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jsdw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niklasad1 Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

lexnv Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

jsdw Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsdw Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsdw commented Mar 5, 2024

Uh oh!

Uh oh!

niklasad1 Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

lexnv Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lexnv commented Feb 21, 2024 •

edited

Loading

niklasad1 left a comment •

edited

Loading

jsdw Mar 4, 2024 •

edited

Loading

niklasad1 Mar 4, 2024 •

edited

Loading

niklasad1 commented Mar 5, 2024 •

edited

Loading