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

rpc module: simplify error handling in subscriptions #1008

Closed
niklasad1 opened this issue Feb 8, 2023 · 3 comments
Closed

rpc module: simplify error handling in subscriptions #1008

niklasad1 opened this issue Feb 8, 2023 · 3 comments

Comments

@niklasad1
Copy link
Member

Currently, jsonrpsee requires the subscription async block to return an error which is just used to log errors.

It would be better if these could be handled by users in simpler API.

Originally posted by @jsdw in #962 (comment)

@niklasad1
Copy link
Member Author

niklasad1 commented Feb 16, 2023

In essence, we could send a custom close message when this fails or terminated but is this is heavily depending on the use-case and that's the reason jsonrpsee doesn't send out this message. It's possible to manually send out on ordinary notification or close by the APIs but easy to shot oneself in the foot and return early from the closure.

As we have discussed it could be possible with these options:

  1. Add a closure to register_subscription to provide a default close value and/or use the close reason as error (this is tricky because it would need to be added to the proc macro API as well)
  2. Refactor the subscription callback to return Option<Result<SubscriptionMessage, SubscriptionMessage>> (None: no notification is sent, Some(Ok(m)): a notification with the result field is sent, Some(Err(e)): a notification with the error field is sent

Option is 2) is simple to implement but it's reduces the ergonomics.

	let mut module = RpcModule::new(());
	module
		.register_subscription("sub", "_", "unsub", |_, pending, _| async move {
			// this just return `None` i.e, no extra notification is sent
                        // the connection is most likely closed
			let sink = pending.accept().await.ok()?;
                        
                        // if the JSON value couldn't be serialized sent out an error notification
		        let msg = SubscriptionMessage::from_json(&json).map_err(|e| Some(SubscriptionMessage::from_str(&e.to_string()))?;
			 
                        // if send fails here the connection is closed, no need to send an notification
                        // return None here 
                        sink.send(msg).await.ok()?;
	
                        // Send a regular subscription notification closed 
			Some(Ok(SubscriptionMessage::from_str("Closed")))
		})
		.unwrap();

@niklasad1
Copy link
Member Author

//cc @lexnv @jsdw what do you guys think about Option 2)?

It's a bit awkward but it's the best I could come up with.

@niklasad1
Copy link
Member Author

Closed by #1034

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant