-
Notifications
You must be signed in to change notification settings - Fork 173
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
fix(rpc module): failed unsubscribe
should be logged in middleware
#792
Conversation
@@ -311,3 +315,46 @@ async fn subscribing_without_server_bad_params() { | |||
matches!(sub, Error::Call(CallError::Custom(e)) if e.message().contains("invalid length 0, expected an array of length 1 at line 1 column 2") && e.code() == ErrorCode::InvalidParams.code()) | |||
); | |||
} | |||
|
|||
#[tokio::test] | |||
async fn subscribe_unsubscribe_without_server() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is more or less copied from https://github.com/paritytech/substrate/blob/master/client/beefy/rpc/src/lib.rs#L245-#L269
I have tried to debug it but doesn't hurt to have that test here as well
@@ -60,7 +60,7 @@ pub type AsyncMethod<'a> = Arc< | |||
dyn Send + Sync + Fn(Id<'a>, Params<'a>, MethodSink, ConnectionId, Option<ResourceGuard>) -> BoxFuture<'a, bool>, | |||
>; | |||
/// Method callback for subscriptions. | |||
pub type SubscriptionMethod = Arc<dyn Send + Sync + Fn(Id, Params, &MethodSink, ConnState) -> bool>; | |||
pub type SubscriptionMethod = Arc<dyn Send + Sync + Fn(Id, Params, MethodSink, ConnState) -> bool>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to indicate that the subscription takes ownership of the sink
it was cloned under the hood before, this should make it clear that it will be alive as long as the subscription callback is alive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.