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: avoiding IDs override/conflict while creating concurrent runtime notifications #342

Merged

Conversation

zeeshanakram3
Copy link
Contributor

Context

While creating the runtime notifications addRuntimeNotification function reads the nextEntityId for the new notification from the db. And, then use this id to create a new entity in the overlay.

Now the problem is that if many concurrent notifications are being created e.g. Promise.all([...]). Then each concurrent call of addRuntimeNotification will read the same next entity ID from the db, and only one notification can be created.

Fix

Instead of using db to get the next entity ID, read the ID form the overlay and then update it too, so the each concurrent addRuntimeNotification instance has access to the correct next entity ID.

@zeeshanakram3 zeeshanakram3 requested a review from ikprk July 16, 2024 22:08
const id = parseInt(row?.nextId.toString() || '1')

// Update the id to be the next one in the overlay
if (row) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

where did we increment the id for entity previously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the addRuntimeNotification function

Copy link
Collaborator

@ikprk ikprk left a comment

Choose a reason for hiding this comment

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

Not sure whether I get the logic, previously we invoked getNextIdForEntity for multiple notifications and then incremented the row. Now we will increment it when each notification is being created, this way there arent conflicts?

@zeeshanakram3
Copy link
Contributor Author

Not sure whether I get the logic, previously we invoked getNextIdForEntity for multiple notifications and then incremented the row. Now we will increment it when each notification is being created, this way there arent conflicts?

No, there wont be any conflicts, because we are incrementing the ID and then reading it from the overlay (which is in-memory cache) and not db. So there is no asynchronous operation involved in update the id.

@zeeshanakram3 zeeshanakram3 merged commit 3373b62 into Joystream:master Jul 17, 2024
7 checks passed
malchililj added a commit to malchililj/orion that referenced this pull request Sep 3, 2024
… notifications (Joystream#342)

* fix: avoiding IDs override while creating conturrent runtime notifications

* create record in 'getNextIdForEntity' function in overlay for entityname, if it does not exist

* fix: notifications unit tests
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.

2 participants