Add resource limiting for `Subscriptions` #786

lexnv · 2022-05-30T16:29:10Z

This PR ensures that resource limiting is available for Subscriptions.

The ResourceGuard wrapper is kept alive for as long as a SubscriptionSink is utilized to properly
increment and decrement the internal resources. Otherwise, the resource limiting would have the
same effect as for sync methods, which would acquire the resources and drop them immediately
after the function was called.

This implies that subscription registration behaves like normal methods, returning a resource-builder
that can accept limits. To ensure proper feature exposure, the proc-macro generation is extended
to receive resources while generating subscriptions.

While at it, extend the resource limiting tests with the subscription limit.

Closes #766 .

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Registering a subscription returns the subscription' callback wrapped into a `MethodResourcesBuilder` for resource limiting purposes. Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

niklasad1 · 2022-05-30T16:49:44Z

core/src/server/rpc_module.rs

@@ -689,39 +689,14 @@ impl<Context: Send + Sync + 'static> RpcModule<Context> {
 			return Err(Error::SubscriptionNameConflict(subscribe_method_name.into()));
 		}

-		self.methods.verify_method_name(subscribe_method_name)?;


can you explain this?

these are still required AFAIU, if any of the method names are already registered we should still fire an error

Good question, I've utilized the verify_and_insert method, which should still return error if the method is already registered. Given I've needed the callback reference back for subscription, I've picked the same registering pattern for subscription as well 😄

The problem is that you check the subscription and unsubscription indiviudally.
Thus, if only the unsubscription fails we should still deny both.

For example, if the subscription is successful and the unsubscription fails you can't really unsubscribe 🤔

EDIT: Ok, I saw that you changed so unsubscriptions are performed first which "should" work but a bit "unexplicit" :)

I see now what you mean! Indeed this was living the door open for having the unsubscriptions left initialized, have modified to use the previous way of first verifying the method's name. Good catch! Thanks!

niklasad1

Looks overall good.

I don't remember exactly but this is tricky because often these subscriptions might be event based or periodic.

So once I register a subscription it's not really "static" as it depends on how often notifications are sent out or whether a given state has been triggered.

So the question is really whether the SubscriptionSink also should be part of the resource guard. I reckon that it might quite hard to reason about once one "register resources" on a subscription.

For example take state_subscribeStorage which depends heavily on the number of storage keys which is a parameter to the subscription, it's impossible to give a accurate static resource limit without knowing the number of storage keys. I suppose the amount data sent via the SubscriptionSink could be one way to determine that.

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

ws-server/src/server.rs

jsdw · 2022-06-13T09:40:36Z

At the retreat it sounded like you guys decided now to add resource limiting for subscriptions owing to various things? Does that render this PR obsolete?

niklasad1 · 2022-06-13T09:51:50Z

At the retreat it sounded like you guys decided now to add resource limiting for subscriptions owing to various things? Does that render this PR obsolete?

Well, we have discussed this and implementing resource limiting on subscriptions properly with the "abstract user defined metrics" is just too hard.

The idea is to implement/investigate to do backpressure on the sink instead and kill resource limiting completely.

jsdw · 2022-06-13T10:49:26Z

Is this PR still valid and wants to be merged then? Wondering whether to look over it or not :)

niklasad1 · 2022-06-13T12:18:56Z

We can merge it anyway but as I pointed it out it won't well with "dynamic keys" but as long as we have resource limiting the PR makes sense.

jsdw · 2022-06-14T11:14:01Z

tests/tests/resource_limiting.rs

+	// 3 CPU units (manually set for subscriptions) per call, the sink is dropped immediately
+	let (pass1, pass2, pass3) = tokio::join!(
+		client.subscribe::<i32>("subscribe_hello", None, "unsubscribe_hello"),
+		client.subscribe::<i32>("subscribe_hello", None, "unsubscribe_hello"),
+		client.subscribe::<i32>("subscribe_hello", None, "unsubscribe_hello"),
+	);


Am I right in thinking that even if there were 10 client.subscribes here, they would all be OK because something is dropped immediately, and there is no race condition where that might not always be true?

(if so, could the comment be tweaked to say something like "the sink is dropped immediately, so no resources are claimed and all of the calls succeed"?"

I believe that to be the case for this particular test.

We submit all the subscribe requests from one client to the server. The server handles each client on a dedicated background_task task. That means all our submitted subscribe requests get handled serially, on the same background task.
The subscription handling would: acquire + update + release resource table, call subscription callback, drop the sink immediately, acquire + update + release resource table.

The resource limiting would reject connections for multiple clients if they attempt to make a subscription call to a method that drops the sink immediately.

jsdw · 2022-06-14T11:15:31Z

ws-server/src/tests.rs

@@ -21,7 +21,7 @@
 // SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
 // CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
 // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR
-// IN background_task WITH THE SOFTWARE OR THE USE OR OTHER
+// IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER


jsdw

Looks great to me offhand! I do think that having a consistent resource limiting API across everything is good, and at the very least it allows subscriptions to say "this is the minimum amount of CPU/memory I need available in order to run", even if some subscriptions may use more (it reminds me of resources in kubernetes pods, where you can't know how much they could use, but providing some idea is better than not at all).

Given that you can have a "SUB" resource as in thexe examples, and use it to limit the max subscriptions, I wonder whether this will eventually lead to the explicit "max subscriptions" feature being removed (or at least rendered less useful, since this API is more powerful)?

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv added 7 commits May 30, 2022 15:26

ws-server: Fix copyright for tests

b270777

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

rpc_module: Return a resource builder when subscribing

c32742d

Registering a subscription returns the subscription' callback wrapped into a `MethodResourcesBuilder` for resource limiting purposes. Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tests: Fix register_subscription tests

b82445e

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

server: Drop ResourceGuard with SubscriptionSink for resource limit

c8fb960

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tests: Check resource limits for subscription

058084c

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

proc-macros: Render resource limits for subscription macro

9815a4c

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

tests: Extend subscription limiting test via macro generation

8b3cbef

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv requested a review from a team as a code owner May 30, 2022 16:29

lexnv self-assigned this May 30, 2022

lexnv requested a review from niklasad1 May 30, 2022 16:31

core: Check if the unsubscribe method was already inserted

2bd9574

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

niklasad1 reviewed May 30, 2022

View reviewed changes

lexnv added 2 commits May 31, 2022 13:29

tests: Fix unsupported fields for subscriptions

0d055c0

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

server: Verify subscription methods before registering them

47221c5

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

niklasad1 reviewed May 31, 2022

View reviewed changes

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

niklasad1 approved these changes May 31, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into 766_limit_subscription

de93142

jsdw reviewed Jun 14, 2022

View reviewed changes

jsdw approved these changes Jun 14, 2022

View reviewed changes

lexnv added 2 commits June 14, 2022 14:59

Update test comment for subscription limiting

039abcb

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Modify tests comments

0396572

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

lexnv merged commit 01577da into master Jun 14, 2022

lexnv deleted the 766_limit_subscription branch June 14, 2022 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource limiting for `Subscriptions` #786

Add resource limiting for `Subscriptions` #786

lexnv commented May 30, 2022

niklasad1 May 30, 2022 •

edited

Loading

lexnv May 30, 2022

niklasad1 May 30, 2022 •

edited

Loading

lexnv May 31, 2022

niklasad1 left a comment •

edited

Loading

jsdw commented Jun 13, 2022

niklasad1 commented Jun 13, 2022 •

edited

Loading

jsdw commented Jun 13, 2022

niklasad1 commented Jun 13, 2022

jsdw Jun 14, 2022

jsdw Jun 14, 2022

lexnv Jun 14, 2022

jsdw Jun 14, 2022

jsdw left a comment

Add resource limiting for Subscriptions #786

Add resource limiting for Subscriptions #786

Conversation

lexnv commented May 30, 2022

niklasad1 May 30, 2022 • edited Loading

Choose a reason for hiding this comment

lexnv May 30, 2022

Choose a reason for hiding this comment

niklasad1 May 30, 2022 • edited Loading

Choose a reason for hiding this comment

lexnv May 31, 2022

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

jsdw commented Jun 13, 2022

niklasad1 commented Jun 13, 2022 • edited Loading

jsdw commented Jun 13, 2022

niklasad1 commented Jun 13, 2022

jsdw Jun 14, 2022

Choose a reason for hiding this comment

jsdw Jun 14, 2022

Choose a reason for hiding this comment

lexnv Jun 14, 2022

Choose a reason for hiding this comment

jsdw Jun 14, 2022

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

Add resource limiting for `Subscriptions` #786

Add resource limiting for `Subscriptions` #786

niklasad1 May 30, 2022 •

edited

Loading

niklasad1 May 30, 2022 •

edited

Loading

niklasad1 left a comment •

edited

Loading

niklasad1 commented Jun 13, 2022 •

edited

Loading