Skip to content

Conversation

@svyatonik
Copy link
Contributor

When unsubscribe request is sent, the send_back is None (see here). When response to this request is received, the request id is not reclaimed by the request manager (see this and this) => if app starts and drops subscriptions, it'll end up with MaxSlotsExceeded error. Like it already happened in paritytech/parity-bridges-common#909 .

Unfortunately, I'm unable to compile master branch of jsonrpsee with latest stable rust => can't run tests and see if this patch breaks something. But at least applying this patch to the alpha.4 solves our issue.

Copy link
Contributor

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice catch

However, when #269 when/if merged this logic will be simplified

@niklasad1
Copy link
Contributor

niklasad1 commented Apr 19, 2021

Unfortunately, I'm unable to compile master branch of jsonrpsee with latest stable rust => can't run tests and see if this patch breaks something. But at least applying this patch to the alpha.4 solves our issue.

Hmm, really? Please open issue

@svyatonik
Copy link
Contributor Author

svyatonik commented Apr 19, 2021

Yeah - I'm getting this when trying to run cargo test --all. Removing target also doesn't help - I'm not sure what else I could do :)

UPD: actually removing Cargo.lock has helped.

@niklasad1 niklasad1 merged commit de7b58a into master Apr 19, 2021
@niklasad1 niklasad1 deleted the return-request-id-on-unsubscribe branch April 19, 2021 10:48
}

#[tokio::test]
#[ignore]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this about, are these tests failing even with this fix?

Copy link
Contributor

@niklasad1 niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the nodes have been down so ignored those for now ^

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

Successfully merging this pull request may close these issues.

3 participants