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

Remove RepositoryManager RwLock #1007

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Conversation

timbru
Copy link
Contributor

@timbru timbru commented Feb 1, 2023

[draft] We should make a branch for prep-0.12.2 and merge to that if this is stable.

This PR removes the RwLock in RepositoryManager.

We saw issues where a busy Krill Publication Server would freeze without any reported issues. We suspect that this may be due to a deadlock or lock starvation issue in RepositoryManager. A version where read locks were no longer acquired (they are actually not needed) - was deployed and so far seems stable.

Then one more commit was done to remove the RwLock altogether in this layer. It is in fact not needed because there are existing RwLocks in the layers below this (WalStore::send_command) to ensure that actual updates do happen sequentially.

Tim Bruijnzeels added 3 commits January 31, 2023 16:23
This should not be needed. And it might lead to write lock starvation.
Typically cliens would call list before sending updates and not send
two requests at the same time. In case we don't have the read lock then
the content may not contain an update for another publisher, but that
is irrelevant here.
@timbru timbru marked this pull request as draft February 1, 2023 09:02
@Koenvh1
Copy link
Contributor

Koenvh1 commented Feb 2, 2023

I can understand why the previous deadlock happened, but I am not certain something similar could not still happen with the current locks in the WalStore. Let me see if I can create a test for that somehow.

@timbru timbru marked this pull request as ready for review February 9, 2023 10:25
Copy link
Member

@ximon18 ximon18 left a comment

Choose a reason for hiding this comment

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

Approved following a deep dive in the code and several discussions with @tim.

Copy link
Contributor

@Koenvh1 Koenvh1 left a comment

Choose a reason for hiding this comment

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

As far as I can see this should not cause any issues

@timbru timbru changed the base branch from main to prep-0.12.2 February 20, 2023 09:45
@timbru timbru merged commit 592ed05 into prep-0.12.2 Feb 20, 2023
@timbru timbru deleted the publisher-list-read-lock branch February 20, 2023 15:25
timbru pushed a commit that referenced this pull request Feb 21, 2023
timbru pushed a commit that referenced this pull request Feb 21, 2023
timbru pushed a commit that referenced this pull request Mar 3, 2023
timbru pushed a commit that referenced this pull request Mar 6, 2023
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.

3 participants