fix(rpc module): fail subscription calls with bad params

[niklasad1]

To summarize, with this PR:

• We can reject a pending subscription with a { jsonrpc: "2.0", error: {...}, "id":<ID> } style error of our choice using pending.reject(error).
• We can end a subscription at any time with { jsonrpsee: "2.0", method: "subscribe_foo", params: { subscription: "123abc", error: { ... } } } style error using sink.close(error).
• Subscriptions that are dropped are now not silently closed with an error anymore (for compatibility reasons, incase subscriptions want to signal that they got closed by: "completed successfully" or "error" they must either call close on SubscriptionSink explictly). In other words, we enforce no opinion on what happens when a subscription is closed, and leave it up to the subscription itself to decide whether to emit an "error" or communicate closure via some other "ordinary notification".
• The clients have been changed to require the new error format for closing subscriptions { jsonrpsee: "2.0", method: "subscribe_foo", params: { subscription: "123abc", error: { ... } } }
• Subscription::next in the client no longer parses SubscriptionClosed instead just returns None when a closed message is received.

Tested it on: https://github.com/paritytech/substrate/compare/dp-jsonrpsee-integration-2...na-jsonrpsee-comp-bad-params?expand=1

[niklasad1]

unfortunately, this will return a subscription ID whether to call was actually successful or not!!

and we can't do this after the callback because then subscription sink might already have started sending out stuff on the subscription which is no go. That will break everything.

so indeed we need the users manually to call accept or reject the subscription whether call was ok after parsing the params. we can't do it because it depends on their callback.

so I introduced PendingSubscription that you need to call reject or accept on which should work.

[jsdw]

I'm trying to understand; under which circumstances would the callback not run PendingSubscription::accept()? Is this so that a provided subscription callback can do arbitrary processing in order to then decide that theere is some issue and we don't want to "begin" the subscription but instead want to return an error (eg parameters aren't what we expected)?

[jsdw]

(that being the case; I had a skim over and I like the PendingSubscription stuff; it's a nice way to ensure that the thing is either rejected or accepted before any messages are sent :))

[niklasad1]

I'm trying to understand; under which circumstances would the callback not run PendingSubscription::accept()

It's a way to ensure if params.parse() fails then actual subscription is denied so my initial idea was that the proc macro should this under the hood but as substrate and some other applications might want to deny based on there needs it's better to leave it up to the user I guess even if it's annoying to call accept and reject manually.

[jsdw]

So, if I understand it right, there are basically 2 places an error can happen with a subscription;

1. Before it is accepted (ie the params aren't valid, or some other thing that means it's not valid to ask for said subscription (eg server not configured to allow it or something)). Here, we never want to give back a subscription ID etc and want to bail with something like an { jsonrpc: "2.0", error: { code: 123, message: "Invalid params" }} style message, I guess?
2. During the subscription. Some error could occur mid-subscription that, I assume, means we want to end the subscription and report the error back (would we always want to end the subscription on an error?). I guess in this case we want to return a message like { jsonrpsee: "2.0", method: "subscribe_foo", params: { subscription: "123abc", error: { code: 124, message: "Something went wrong" } } }.

I guess this change allows for (1). Is there a way for people to do (2)?

We also want a way to signal that the subscription has closed without an error. I guess the options for this are:

1. Return an error (like in (2) above) with a code/message that signals the thing has closed.
2. Invent more syntax like params: { subscription: "123abc", closed: true } to signal this.
3. Leave it up to users to interpret a certain non-error response as meaning that no further messages will be yielded.

How did JSONRPC handle this sort of thing? Are we leaning towards doing (1)?

[niklasad1]

So, if I understand it right, there are basically 2 places an error can happen with a subscription;

Exactly.

Before it is accepted (ie the params aren't valid, or some other thing that means it's not valid to ask for said subscription (eg server not configured to allow it or something)). Here, we never want to give back a subscription ID etc and want to bail with something like an { jsonrpc: "2.0", error: { code: 123, message: "Invalid params" }} style message, I guess?

Yes, before a subscription call with invalid params was replied to twice because of some of bug in the logic. Because the actual callback (user defined) we can't know whether the params were valid or not unless we could pause the mpsc channel but that seems like a bad idea. So instead we have this accept/reject API that user has to use. Additionally I have a drop impl for this PendingSubscription so the actual call will be rejected if this is dropped.

During the subscription. Some error could occur mid-subscription that, I assume, means we want to end the subscription and report the error back (would we always want to end the subscription on an error?). I guess in this case we want to return a message like { jsonrpsee: "2.0", method: "subscribe_foo", params: { subscription: "123abc", error: { code: 124, message: "Something went wrong" } } }.

I guess this change allows for (1). Is there a way for people to do (2)?

Yes, there is the close API where folks can close down the subscription using the jsonrpsee::Error.
Before, jsonrpsee closed down subscription and sent out this under the hood by I have realized that this may not be needed for some use cases such as if subscription method itself has defined some state machine or something for this themselves then we force this overhead on them. Also not break comparability with some libraries "may" not support this.

We also want a way to signal that the subscription has closed without an error. I guess the options for this are:

Return an error (like in (2) above) with a code/message that signals the thing has closed.
Invent more syntax like params: { subscription: "123abc", closed: true } to signal this.
Leave it up to users to interpret a certain non-error response as meaning that no further messages will be yielded.

Currently, we treat "closed" as an error on send out the same format with some other status code that we pre-defined:

Notification { jsonrpc: TwoPointZero, method: "n", params: SubscriptionPayloadError { subscription: Num(60482361782361), error: Object({"code": Number(-32003), "message": String("Subscription was closed by server: Success")}) }

So that's is a bit annoying but I think that's reasonable workaround not to break comparability with JS for now, however this now possible to opt in/out from one needs to call close explicitly now:

let _ = sink.pipe_from_try_stream(stream).await.map_err(|e| sink.close(e));

How did JSONRPC handle this sort of thing? Are we leaning towards doing (1)?

JSON-RPC doesn't support close notifications AFAIU but it has (1) similar to what is implemented in this PR.
So I think it relies on the impl to have proper definitions on the different states of a subscription (if you look at "author_submitAndWatchExtrinsic" it has the states of TransactionStatus) when to close the subscriptions using "ordinary notifications". So this is probably not needed for substrate but I still think it's nice feature to have

[jsdw]

Thank you, that's really useful info!

To summarize, with this PR:

• We can reject a pending subscription with a { jsonrpc: "2.0", error: {...} } style error of our choice using pending.reject(error).
• We can end a subscription at any time with { jsonrpsee: "2.0", method: "subscribe_foo", params: { subscription: "123abc", error: { ... } } } style error using sink.close(error).
• Subscriptions that are dropped are now silently closed (for compatibility reasons, incase subscriptions want to signal that they are closed via the "success" messages and not via a "subscription closed" error). In other words, we enforce no opinion on what happens when a subscription is closed, and leave it up to the subscription itself to decide whether to emit an "error" or communicate closure via some other message.

I think if I've understood all of that right, it sounds good to me! The more we stay compatible with how jsonrpc works, the easier!

[niklasad1]

yeah, I think that should correct after the latest changes I pushed for a short while ago.

The only thing is that the subscription API is bit verbose now to work with but it should work.
It might even make sense to do the jsonrpc approach and implement SinkExt on the SubscriptionSink to not have to deal with all this.

[jsdw]

Looking good to me now!

I like that users have a choice on how to signal subscription closing and when/how to report errors. It does make some code a bit more verbose, but if that gets annoying, we can improve on it in the future.

[jsdw]

Just a random thought that came to mind (as a future ergonomic improvement and not something that needs to be worked on now):

Currently, subscriptions accept a pending thing to accept() or reject(), and then once accepted they get a sink to send messages or an error back on. Closures don't return a Result` any more because it isn't obvious what that would mean (does it send an error, and if so, what sort of error?)

One alternate API might be to have the old API of giving subscription closures a sink immediately (rather than a pending), and allowing them to return Result<(), ErrorObject>s, but to keep track of whether the sink has been used or not internally, so that:

• If the closure returns with an error but the sink hasn't been used yet, close the subscription and return that ErrorObject to the client as a normal jsonrpc error (eg { jsonrpc: 2.0, error: { message, code, data } })
• If the closure returns with an error and the sink has been used, then close the subscription and return the ErrorObject as a { jsonrpc: 2.0, params: { subscription: "abc", error: { message, code, data } }}.

With that API, it might be a bit more ergonomic handling errors (no need to match on each one and send it through the sink.close method), and no explicit pending.accept() needed; just bail with an error before sending a message if params are invalid or whatever. The downside might also be that it is less explicit about how exactly an error will be communicated back, and that it is easier to "accidentally" send an error back to the client because ? was used somewhere. Just thinking out loud really :)

[niklasad1]

So, I think we still need a way for the user to tell when to generate the subscriptionID and reply to the subscribe_call either way (I guess we could technically generate for every call and use the error notification to indicate errors but seems like wrong thing to do)

so we could actually kill the PendingSubscription type and have method accept/assign_id/reply to subscribe call that internally has Option<SubscriptionId>.

If ☝️ is None => return ErrorObject with ID else => return ErrorObject notification

but maybe jsonrpc way is better where they did some magic stuff to convert the stream into a SinkExt then the errors can be handled directly by the user

[jsdw]

Ah yup, it does make sense that the subscription can decide when to generate the ID and send off that initial subscription ID message separately from when it'll want to send any subscription notifications, and the PendingSubscription API is really nice for making it explicit when each of these things happens.

So yup, I'm up for merging this anyway, and we can ponder ways to make the API a little more ergonomic in the future :)

[dvdplm]

When the server shuts down, which of these would be the sent? Success? If yes, maybe we should tweak the docs to mention that?

[niklasad1]

no, Success is only used when the stream was completed i.e, returned Ok(None) all other server related errors will be treated as Failed.

However, if the send fails we might regard it as AbortedByRemotePeer when it gets TrySendError(Disconnected) technically the server could have been closed entirely but at that point nothing can sent anyway so should ok.