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

Fix leak in stream notifications #5739

Merged
merged 4 commits into from
Apr 23, 2020
Merged

Conversation

gnunicorn
Copy link
Contributor

@gnunicorn gnunicorn commented Apr 22, 2020

When an rpc client connects to a node, it subscribes to a few keys to receive update when they change. Internally that is offloaded to client.notification_stream, which holds the actual logic to figure out if it matches and trigger an event on an unbounded channel if it does.

When the RPC disconnects all these subscriptions are discarded, the Receivers drop, which would mean the notification stream will also drop them on the next trigger that matches them – eventually cleaning it up internally.

Well, that's the theory. But because it only checks for the dropped ones whenever they match, rarely (or effectively never) changing keys are not included in the check. And one of those rarely changing keys is one the Polkadot JS App is very interested in: the runtime version. Thus, whenever a client connects via rpc and subscribes to that (which it does at start up), it is added to the stream notifications, yet effectively never removed. Leading to us accumulating stale sinks that are never cleaned up.

This PR fixes this leak by iterating through all sinks on every trigger and remove those that not connected anymore. Thus we clean this upon effectively ever trigger cycle.

Note: A cleaner way to resolve this issue would be to hand over the subscriber ID to the outside of stream notifications and provide an API to actively remove subcribers rather than relying on the implic drop to take place. So that the RPC could do that right on the connection drop (as it does for its own internal subscription system). But that is a more involved change that we don't want to do at the moment.

Todo:

  • fix leak
  • add metrics to prevent this from happening again

@gnunicorn gnunicorn requested a review from tomusdrw April 22, 2020 17:08
@gnunicorn gnunicorn added A0-please_review Pull request needs code review. B1-clientnoteworthy labels Apr 22, 2020
@gnunicorn gnunicorn marked this pull request as ready for review April 22, 2020 17:09
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

@gnunicorn gnunicorn added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 23, 2020
@gnunicorn gnunicorn merged commit 1c4f001 into master Apr 23, 2020
@gnunicorn gnunicorn deleted the ben-fix-stream-notifications-leak branch April 23, 2020 10:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants