1838
Comment:
|
← Revision 3 as of 2010-08-16 10:24:09 ⇥
1869
converted to 1.6 markup
|
Deletions are marked like this. | Additions are marked like this. |
Line 26: | Line 26: |
|| 151 || One might comment here or elsewhere, that we're trying to make an exclusive lock on this FileChannel. A FileLock can also be shared. || Cosmetic || NOTOK || || 159 || Findbugs gives the following warning here: the double-checked locking idiom is broken and should be avoided. That's interesting, since we don't use the double-checked locking idiom in any way. Wait, maybe we do... Okay, we could drop lines 133-135 and only check file existence within the synchronized block. Actually, this is probably required unless cacheData guarantees that the creation of cachedFile is atomic... In our given implementations, that is the case (we use renaming of the finished data), but since this is a framework method, we should probably sacrifice what little performance is gained here to ensure validity of code. I don't suggest that for the production fix, though. Whoops, no we don't even do that. This is actually a bug where you can end up delivering a partially generated cache! I _do_ recommend implementing this for the production fix. || Major || NOTOK || |
|| 151 || One might comment here or elsewhere, that we're trying to make an exclusive lock on this FileChannel. A FileLock can also be shared. || Cosmetic || Rejected (already mentioned in javadoc) || || 159 || Findbugs gives the following warning here: the double-checked locking idiom is broken and should be avoided. That's interesting, since we don't use the double-checked locking idiom in any way. Wait, maybe we do... Okay, we could drop lines 133-135 and only check file existence within the synchronized block. Actually, this is probably required unless cacheData guarantees that the creation of cachedFile is atomic... In our given implementations, that is the case (we use renaming of the finished data), but since this is a framework method, we should probably sacrifice what little performance is gained here to ensure validity of code. I don't suggest that for the production fix, though. Whoops, no we don't even do that. This is actually a bug where you can end up delivering a partially generated cache! I _do_ recommend implementing this for the production fix. || Major || OK || |
Review (NS-86): Bug 1547 in stable branch: Synchronize entire block that locks files during index caching
Author |
Kåre |
Moderator |
Kåre |
State |
Closed |
Objectives
It is probably a good idea to read up on the FileLock javadoc as part of this review
Summary
KFC follows up
Total Time Used (Coding,Documentation,Review):
Time use (Coding,Documentation,Review) KFC: 0.5,0,0.5 SVC: 0.0,0.0,0.5
General comments:
Description |
Classification |
Status |
Comments on file 'branches/3.8.0/src/dk/netarkivet/archive/indexserver/FileBasedCache.java', revision 990
Lines |
Description |
Classification |
Status |
151 |
One might comment here or elsewhere, that we're trying to make an exclusive lock on this FileChannel. A FileLock can also be shared. |
Cosmetic |
Rejected (already mentioned in javadoc) |
159 |
Findbugs gives the following warning here: the double-checked locking idiom is broken and should be avoided. That's interesting, since we don't use the double-checked locking idiom in any way. Wait, maybe we do... Okay, we could drop lines 133-135 and only check file existence within the synchronized block. Actually, this is probably required unless cacheData guarantees that the creation of cachedFile is atomic... In our given implementations, that is the case (we use renaming of the finished data), but since this is a framework method, we should probably sacrifice what little performance is gained here to ensure validity of code. I don't suggest that for the production fix, though. Whoops, no we don't even do that. This is actually a bug where you can end up delivering a partially generated cache! I _do_ recommend implementing this for the production fix. |
Major |
OK |