Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Automatically unsubscribe storage listeners when they're dropped (RCP node memory leak fix) #10454

Merged

Conversation

koute
Copy link
Contributor

@koute koute commented Dec 9, 2021

Any subscription to a key which doesn't exist (hence the subscription will never be triggered) results in a memory leak. This PR fixes the issue by simply automatically unsubscribing the listener when the event channel through which the events are supposed to be sent is dropped.

I've also added a trace! log which we can enable on one of the RPC nodes and see which exact keys are not being triggered.

This should also fix the leak on the jsonrpsee-based nodes, as the underlying issue is the same AFAIK.

(I've tagged this as B5-clientnoteworthy since it fixes an issue which also affected third-party RPC nodes, so it might be worthwhile to mention in the public changelog that this memory leak was fixed. Feel free to downgrade to B0-silent if you disagree.)

cc FYI @tomusdrw @niklasad1 @maciejhirsz

@koute koute added A0-please_review Pull request needs code review. I3-bug The node fails to follow expected behavior. I8-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 9, 2021
@koute koute requested review from gilescope and bkchr December 9, 2021 09:06
@dvdplm
Copy link
Contributor

dvdplm commented Dec 9, 2021

We're running this + jsonrpsee on kusama-rpc-3 now, grafana here comparing it to kusama-rpc-4 running master (the CPU spike is rustc recompiling the binary).

Looks good so far! :)

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

LGTM

client/api/src/notifications.rs Outdated Show resolved Hide resolved
client/api/src/notifications.rs Outdated Show resolved Hide resolved
client/api/src/notifications.rs Show resolved Hide resolved
client/api/src/notifications.rs Outdated Show resolved Hide resolved
client/api/src/notifications.rs Outdated Show resolved Hide resolved
client/api/src/notifications.rs Outdated Show resolved Hide resolved
client/api/src/notifications.rs Outdated Show resolved Hide resolved
@koute
Copy link
Contributor Author

koute commented Dec 10, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit be75e55 into paritytech:master Dec 10, 2021
seunlanlege pushed a commit to seunlanlege/substrate that referenced this pull request Dec 17, 2021
… node memory leak fix) (paritytech#10454)

* Automatically unsubscribe storage listeners when they're dropped

* Fix tests' compilation in `sc-client-api`

* Add an extra test

* Align to review comments; cleanups

* Update client/api/src/notifications.rs

* FMT

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
… node memory leak fix) (paritytech#10454)

* Automatically unsubscribe storage listeners when they're dropped

* Fix tests' compilation in `sc-client-api`

* Add an extra test

* Align to review comments; cleanups

* Update client/api/src/notifications.rs

* FMT

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
… node memory leak fix) (paritytech#10454)

* Automatically unsubscribe storage listeners when they're dropped

* Fix tests' compilation in `sc-client-api`

* Add an extra test

* Align to review comments; cleanups

* Update client/api/src/notifications.rs

* FMT

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit I3-bug The node fails to follow expected behavior. I8-footprint An enhancement to provide a smaller (system load, memory, network or disk) footprint.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants