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

fix: fixed subscriptions for token balances #374

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

peterslany
Copy link
Contributor

@peterslany peterslany commented Jul 11, 2022

In current implementation, we are creating a query subscriptions in the effect hook of the App component to listen for balance changes. Subscription callback functions are depending on the global redux state which they also modify. Therefore a callback has to be modified when the balance state changes. However, we are not able to modify the callback of existing subscriptions and can only create new query subscription with different callback because of the limitations imposed by the polkadot.js api package.

The problem of current implementation is that we are creating new subscription with modified callback with each state change, but are not closing the previous subscriptions. As a result, ever-increasing amount of subscriptions is created with each run of the effect hook which ends up freezing the whole dApp: with each balance change all of these subscriptions are calling the redux reducers which changes the redux state and triggers creation of new subscriptions.

To solve this issue, I moved the callbacks for unsubscribing into React refs and added a call to unsubscribe each time when new subscription is created (not only when the component is unmounted). Also, I split the hook into 3 data-specific ones to enforce that new subscription is created only when related data change.

Note: Ideal solution would be to create subscription on the first render only and then just modify the callback function, but that is not possible now, because the @polkadot/api does not expose this functionality.

@peterslany peterslany requested a review from tomjeatt July 11, 2022 11:02
@vercel
Copy link

vercel bot commented Jul 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
interbtc-ui-interlay ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 11:02AM (UTC)
interbtc-ui-interlay-testnet ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 11:02AM (UTC)
interbtc-ui-kintsugi ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 11:02AM (UTC)
interbtc-ui-kintsugi-testnet ✅ Ready (Inspect) Visit Preview Jul 11, 2022 at 11:02AM (UTC)

Copy link
Collaborator

@tomjeatt tomjeatt left a comment

Choose a reason for hiding this comment

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

Good solution @peterslany - any idea what caused this to become an issue now? It's clearly not something new, so I'm wondering what caused it to become noticeable over the last couple of days.

@peterslany peterslany merged commit 644aac0 into master Jul 11, 2022
@peterslany peterslany deleted the peter/fix-websocket-subscriptions branch July 11, 2022 13:22
@peterslany
Copy link
Contributor Author

Good solution @peterslany - any idea what caused this to become an issue now? It's clearly not something new, so I'm wondering what caused it to become noticeable over the last couple of days.

@tomjeatt I think it became noticeable just because of the testing. Another reason could be that number of the effect hook triggers increased. However I'm not aware of any recent change that could cause this - it would have to be a change to one of the former hook dependencies. :)

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.

None yet

2 participants