Review (NS-86): Bug 1547 in stable branch: Synchronize entire block that locks files during index caching

Objectives

It is probably a good idea to read up on the FileLock javadoc as part of this review

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

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