Resource Limiting #500

maciejhirsz · 2021-10-01T14:44:16Z

Closes #203 (details in the issue).

Remaining TODOs:

proc macros.
HTTP server.

…urce-limiting

… ref to value

utils/src/server/rpc_module.rs

niklasad1 · 2021-10-04T18:08:12Z

ws-server/src/server.rs

@@ -68,9 +70,18 @@ impl Server {
 	}

 	/// Start responding to connections requests. This will block current thread until the server is stopped.
-	pub async fn start(self, methods: impl Into<Methods>) {
+	pub fn start(self, methods: impl Into<Methods>) -> Result<StopHandle, Error> {


niklasad1

looks really promising, I will have a look tomorrow when my head is fresh...

* Squashed macros WIP * bae-less macros! * Make things simpler and more readable * Some comments and DRY aliases parsing * Naming things is hard * Respan is no longer needed * Simpler Arguments * Remove stale code * Apply suggestions from code review Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com> * syn-up all the things, handle resources on methods Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

Co-authored-by: David <dvdplm@gmail.com> Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

niklasad1 · 2021-10-08T13:10:40Z

tests/tests/resource_limiting.rs

+	assert_server_busy(fail_mem);
+
+	// Client being active prevents the server from shutting down?!
+	drop(client);


I think the server will be stopped even if the client is connected but not really important here....

It should do that, but if you remove the drop the test runs forever, so it never actually does shut down 🙃

ah I see, do you mind creating an issue for this?

Yeah, will do

niklasad1 · 2021-10-08T13:22:01Z

utils/src/server/resource_limiting.rs

+impl Resources {
+	/// Register a new resource kind. Errors if `label` is already registered, or if the total number of
+	/// registered resources would exceed 8.
+	pub fn register(&mut self, label: &'static str, capacity: u16, default: u16) -> Result<(), Error> {


capacity a u16 is very flexible and works for all use-cases I think.

However, I wonder if we could provide some more high level method that does something more human friendly as percent (with capacity as 100) or something.

We talked about it with @dvdplm at the retreat. The problem with % is that if you add hardware you can easily reconfigure max capacity, where as going in and adjusting every method to lower cpu or memory % would be quite a hassle.

ws-server/src/server.rs

niklasad1 · 2021-10-08T13:27:44Z

ws-server/src/tests.rs

@@ -59,7 +58,7 @@ async fn server() -> SocketAddr {
 ///     other: `invalid_params` (always returns `CallError::InvalidParams`), `call_fail` (always returns
 /// 			`CallError::Failed`), `sleep_for` Returns the address together with handles for server future
 ///				and server stop.
-async fn server_with_handles() -> (SocketAddr, JoinHandle<()>, StopHandle) {
+async fn server_with_handles() -> (SocketAddr, StopHandle) {


niklasad1

nice work, looks clean as always

tests/tests/resource_limiting.rs

TarikGul

👍

jsdw · 2021-10-11T08:44:30Z

proc-macros/src/attributes.rs

+#[derive(Debug, Clone)]
+pub struct Resource {
+	pub name: syn::LitStr,
+	pub assign: Token![=],


Is this actually needed in the struct (it's just here because you need to parse it I guess)? (I think I can see that it might be cleaner to have here in any case)

Yeah, it's not necessary for anything, but it made both the parse cleaner and the purpose of it more obvious since it maps 1:1 to what you see in the macro use.

jsdw · 2021-10-11T09:02:33Z

types/src/error.rs

+	ResourceNameNotFoundForMethod(&'static str, &'static str),
+	/// Trying to claim resources for a method execution, but the method resources have not been initialized
+	#[error("Method `{0}` has uninitialized resources")]
+	UninitializedMethod(Box<str>),


Bike shedding: I wonder whether something like ResourceUninitializedForMethod might be a touch more consistent with the above resource related (and prefixed) errors?

jsdw

Looks great!

Definitely some of the more readable proc macro code I've come across, too :D

dvdplm

I think the proc_macro example should include resource limiting and the proc macro docs need a fairly chunky piece of docs covering:

updated examples and docs showing the macro syntax alongside the server setup
resources and units are dimensionless (no intrinsic meaning, they mean what you want them to mean)
motivation for setting up resources on the server (as opposed to on the modules), as this creates a somewhat awkward API, where modules declare resources, but the resources themselves are set up and configured when building the server.

(Also, and not introduced in this PR, the proc_macro example prints [2021-10-11T09:56:59Z ERROR jsonrpsee_ws_client::helpers] Dropping subscription Num(8470526752200743) error: TrySendError { kind: Disconnected } when ran with cargo run --example=proc_macro which imo isn't very user friendly)

dvdplm · 2021-10-11T09:07:26Z

proc-macros/src/rpc_macro.rs

+	/// Switch denoting that server trait must be generated.
+	/// Assuming that trait to which attribute is applied is named `Foo`, the generated
+	/// server trait will have `FooServer` name.


Suggested change

/// Switch denoting that server trait must be generated.

/// Assuming that trait to which attribute is applied is named `Foo`, the generated

/// server trait will have `FooServer` name.

/// Switch denoting that the server trait must be generated.

/// Assuming that the trait to which the attribute is applied is named `Foo`, the generated

/// server trait will be named `FooServer`.

dvdplm · 2021-10-11T09:08:11Z

proc-macros/src/rpc_macro.rs

+	/// Switch denoting that client extension trait must be generated.
+	/// Assuming that trait to which attribute is applied is named `Foo`, the generated
+	/// client trait will have `FooClient` name.


Suggested change

/// Switch denoting that client extension trait must be generated.

/// Assuming that trait to which attribute is applied is named `Foo`, the generated

/// client trait will have `FooClient` name.

/// Switch denoting that the client extension trait must be generated.

/// Assuming that the trait to which the attribute is applied is named `Foo`, the generated

/// client trait will be named `FooClient`.

dvdplm · 2021-10-11T09:08:23Z

proc-macros/src/rpc_macro.rs

+	/// Assuming that trait to which attribute is applied is named `Foo`, the generated
+	/// client trait will have `FooClient` name.
+	pub(crate) needs_client: bool,
+	/// Optional prefix for RPC namespace.


Suggested change

/// Optional prefix for RPC namespace.

/// Optional prefix for the RPC namespace.

dvdplm · 2021-10-11T09:12:18Z

tests/tests/resource_limiting.rs

+			sleep(Duration::from_millis(50)).await;
+			Ok("hello memory hog")
+		})?
+		.resource("CPU", 0)?


Add comment explaining that this is the same as omitting the resource, i.e. that 0 is the default?

dvdplm · 2021-10-11T09:15:35Z

ws-server/src/server.rs

@@ -365,6 +396,14 @@ impl Builder {
 		self
 	}

+	/// Register a new resource kind. Errors if `label` is already registered, or if number of
+	/// registered resources would exceed 8.


Please document the meaning of capacity and default.

I wonder if having a ResourceBuilder with .with_cap and .with_default methods would make the api more self-explanatory? Not a blocker but one to ponder.

dvdplm · 2021-10-11T09:21:31Z

ws-server/src/server.rs

@@ -273,7 +300,10 @@ async fn background_task(
 						// complete batch response back to the client over `tx`.
 						let (tx_batch, mut rx_batch) = mpsc::unbounded::<String>();

-						for fut in batch.into_iter().filter_map(|req| methods.execute(&tx_batch, req, conn_id)) {
+						for fut in batch


@maciejhirsz did we get a test&docs for this somewhere?

We had a conversation about this and came to the conclusion that while it's possible to calculate the batch "cost" up front, the semantics would become rather awkward and the overall behaviour of the server a bit hard to understand and reason about; on a busy server a batch could end up in a situation where it never gets enough resources to run or, conversely, a sequentially executed batch might block other requests from running.

maciejhirsz added 30 commits September 14, 2021 13:58

Squash post merge

c0efe3b

RAII claims and some comments

f6684d4

Adding resources to Methods

30ec3f5

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-reso…

b61b110

…urce-limiting

Use arrays for claiming all resources at once

1649dfe

Merge branch 'master' of github.com:paritytech/jsonrpsee into mh-reso…

3071279

…urce-limiting

Register resources on ws-server

4a45611

Return MethodResourcesBuilder from register_method

485780d

Return MethodResourcesBuilder from register_async_method

f62219e

fmt and comments

2e2776d

Add verify_and_insert to get rid of or_insert in order to get mut…

e0b1bec

… ref to value

Resources initialization

2b76b35

No need for ResourceBuilder

23611f8

Comment all the things

bd4e84a

Handle errors on ws-server start method

d61adff

Passing ResourceGuard to method execution

33b2410

Start ws-server in background, simplify all the tests!

f7c53de

Claiming resources on runtime

a10bfd3

Do checked math on resource claiming

b94a89f

Clearer error path for resources at limit

bd837da

Resource limiting tests WIP

bc9b82d

Unbox async futures

d2c84df

fmt

dedf9d4

Cleaner proc macro

3b509cb

Merge branch 'mh-no-box-async' into mh-resource-limiting

c2bc53f

Fix lifetime issues with ResourceGuard with internal Arc

0918715

Remove unnecessary lifetime generic

dfb5de4

Merge branch 'master' into mh-resource-limiting

2ae4709

Working tests

1f4d2d8

Use turbofish in tests

6e95688

niklasad1 reviewed Oct 4, 2021

View reviewed changes

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

niklasad1 reviewed Oct 4, 2021

View reviewed changes

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

niklasad1 reviewed Oct 4, 2021

View reviewed changes

maciejhirsz changed the title ~~[WIP] Resource Limiting~~ Resource Limiting Oct 8, 2021

maciejhirsz and others added 6 commits October 8, 2021 12:35

Resource limiting on the http server

8afa917

Test two resources at once

815efc6

Merge branch 'master' into mh-resource-limiting

e8405e5

Test the HTTP server for resource limiting

abe38a2

Apply suggestions from code review

5e24688

Co-authored-by: David <dvdplm@gmail.com> Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>

Comment tweaks

043d5be

maciejhirsz mentioned this pull request Oct 8, 2021

Reworking attribute macros for resource limiting #507

Merged

maciejhirsz requested a review from TarikGul October 8, 2021 13:00

niklasad1 reviewed Oct 8, 2021

View reviewed changes

ws-server/src/server.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Oct 8, 2021

View reviewed changes

niklasad1 approved these changes Oct 8, 2021

View reviewed changes

maciejhirsz added 3 commits October 8, 2021 14:32

More explicit error type for AttributeMeta::retain

7eea0b3

Remove generic from ResourceTable; derive Default on ws Builder

0a8688b

Make the macros shorter

bc4aba6

TarikGul reviewed Oct 9, 2021

View reviewed changes

tests/tests/resource_limiting.rs Show resolved Hide resolved

TarikGul approved these changes Oct 9, 2021

View reviewed changes

jsdw reviewed Oct 11, 2021

View reviewed changes

jsdw approved these changes Oct 11, 2021

View reviewed changes

dvdplm approved these changes Oct 11, 2021

View reviewed changes

dvdplm merged commit 518a615 into master Oct 11, 2021

dvdplm deleted the mh-resource-limiting branch October 11, 2021 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resource Limiting #500

Resource Limiting #500

maciejhirsz commented Oct 1, 2021 •

edited

Loading

niklasad1 Oct 4, 2021

niklasad1 left a comment

niklasad1 Oct 8, 2021

maciejhirsz Oct 8, 2021

niklasad1 Oct 8, 2021

maciejhirsz Oct 8, 2021

niklasad1 Oct 8, 2021

maciejhirsz Oct 8, 2021

niklasad1 Oct 8, 2021

niklasad1 left a comment

TarikGul left a comment

jsdw Oct 11, 2021 •

edited

Loading

maciejhirsz Oct 11, 2021

jsdw Oct 11, 2021 •

edited

Loading

jsdw left a comment

dvdplm left a comment

dvdplm Oct 11, 2021

dvdplm Oct 11, 2021

dvdplm Oct 11, 2021

dvdplm Oct 11, 2021

dvdplm Oct 11, 2021

dvdplm Oct 11, 2021

	/// Optional prefix for RPC namespace.
	/// Optional prefix for the RPC namespace.

Resource Limiting #500

Resource Limiting #500

Conversation

maciejhirsz commented Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

TarikGul left a comment

Choose a reason for hiding this comment

jsdw Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejhirsz commented Oct 1, 2021 •

edited

Loading

jsdw Oct 11, 2021 •

edited

Loading

jsdw Oct 11, 2021 •

edited

Loading