= 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 ||