Minor fixes to the cache: Deadlock and Perl structure #1353
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To avoid the worker locking itself, the die instruction had to go away. The problem lies in the try_lock_asset, since most workers will wait here and timers start at the same time, there will be a race condition at the point where said function it's called. It could happen also with update_asset method, but it's most unlikely... But if it fails at this point, there's nothing much we can do other than let the job be restarted by the webui
Codecov Report
@@ Coverage Diff @@
## master #1353 +/- ##
==========================================
- Coverage 87.32% 87.29% -0.04%
==========================================
Files 101 101
Lines 7450 7453 +3
==========================================
Hits 6506 6506
- Misses 944 947 +3
Continue to review full report at Codecov.
|
coolo
reviewed
May 30, 2017
| } # purge asset will die anyway in case of failure. | ||
| last if ($cache_real_size < $limit); | ||
| if ($result) { | ||
| foreach my $asset ($result) { |
There was a problem hiding this comment.
to avoid the extra indentation level you can use
for my $asset ($result || ())
okurz
reviewed
May 30, 2017
| @@ -135,7 +135,7 @@ sub download_asset { | |||
| $asset = $code; | |||
| } | |||
| else { | |||
| print $log "CACHE: Abnormal situation, bailing out but still retying\n"; | |||
| print $log "CACHE: Abnormal situation, retry to download\n"; | |||
There was a problem hiding this comment.
Please make this line different from the previous one.
| @@ -251,8 +251,8 @@ sub try_lock_asset { | |||
| }; | |||
|
|
|||
| if ($@) { | |||
| log_error "Rolling back $@"; | |||
There was a problem hiding this comment.
can you also make this different from L215?
| @@ -271,8 +271,8 @@ sub add_asset { | |||
| eval { $dbh->prepare($sql)->execute($asset) or die $dbh->errstr; }; | |||
|
|
|||
| if ($@) { | |||
| log_error "Rolling back $@"; | |||
| @@ -318,8 +318,8 @@ sub purge_asset { | |||
| }; | |||
|
|
|||
| if ($@) { | |||
| log_error "Rolling back $@"; | |||
There was a problem hiding this comment.
wow, so often? please make it also a unique message
| } | ||
| } | ||
| else { | ||
| log_error "There are not more elements to remove!"; |
There was a problem hiding this comment.
s/not more/no more/ ? and isn't this log_warn?
coolo
pushed a commit
that referenced
this pull request
May 30, 2017
commit e5ded23 Merge: d9d39c5 9fe8adc Author: Stephan Kulow <coolo@kde.org> AuthorDate: Tue May 30 20:58:19 2017 +0200 Commit: GitHub <noreply@github.com> CommitDate: Tue May 30 20:58:19 2017 +0200 Merge pull request #1353 from foursixnine/fix/cache_deadlocks Minor fixes to the cache: Deadlock and Perl structure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes poo#19270 and poo#19278