Skip to content

fix: SendInvalidationTrackingMessage should not block. #4680

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

Merged
merged 1 commit into from
Mar 3, 2025
Merged

Conversation

romange
Copy link
Collaborator

@romange romange commented Mar 2, 2025

We call PerformDeletion in an atomic block, which in turn calls SendInvalidationTrackingMessage that could block. We fix it by separating the blocking logic by moving the invalidation messages into a designated send queue and flush it later.

In addition rename the function to make it explicit that they are atomic (i.e. not blocking).

@romange romange requested review from kostasrim and adiholden March 2, 2025 19:25
@romange romange force-pushed the DbSlice branch 3 times, most recently from 11c38df to d3a7caf Compare March 3, 2025 10:15
@adiholden
Copy link
Collaborator

do we have some open github issue related to this fix?

adiholden
adiholden previously approved these changes Mar 3, 2025
@romange
Copy link
Collaborator Author

romange commented Mar 3, 2025

@adiholden no, I just saw it while reading the db_slice code.
I am going to disable test_cluster_sharded_pub_sub - it fails often @kostasrim FYI

We call PerformDeletion in an atomic block, which in turn calls SendInvalidationTrackingMessage
that could block. We fix it by separating the blocking logic by moving the invalidation messages into
a designated send queue and flush it later.

In addition rename the function to make it explicit that they are atomic (i.e. not blocking).

Signed-off-by: Roman Gershman <roman@dragonflydb.io>
@romange romange merged commit 618af31 into main Mar 3, 2025
10 checks passed
@romange romange deleted the DbSlice branch March 3, 2025 12:32
@kostasrim
Copy link
Contributor

kostasrim commented Mar 4, 2025

@adiholden no, I just saw it while reading the db_slice code. I am going to disable test_cluster_sharded_pub_sub - it fails often @kostasrim FYI

Good catch, I made changes around this code through time but I didn't notice it :)

No worries for the pub/sub. On this.

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

3 participants