[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
Re: [jgit-dev] isOutdated() and lastModified()
|
Hello all,
It seems that I've found the reason of the bug.
Here's a part of FileSnapshot#isModified.
// Our lastRead flag may be old, refresh and retry
lastRead = System.currentTimeMillis();
if (notRacyClean(lastRead)) {
return false;
}
lastRead is reported to be "Last wall-clock time the path was read."
But this part of code allows it to be modified without reading ("return false"
means that the path won't be reread).
My experiments have shown that converting it to the local variable doesn't
help. This code also fails.
// Our lastRead flag may be old, refresh and retry
if (notRacyClean(System.currentTimeMillis())) {
return false;
}
Just consider the scenario:
Alice updates the file at the path and records lastModified to her snapshot,
then
Bob updates the same file the same path (with new contents) within the same
granularity period, so the file's File#lastModified has the same value that
Alice's FileSnapshot#lastModified.
After 4 minutes. Alice reads the file. (See FileSnapshow#isModified)
lastModified != currLastModified is false
and notRacyClean(System.currentTimeMillis()) is 100% true.
So FileSnapshow#isModified returns false. Alice won't reread the new contents
of the file.
I can confirm that with this patch the test is passed for me
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileSnapshot.java
@@ -215,12 +215,6 @@ private boolean isModified(final long currLastModified) {
return false;
}
- // Our lastRead flag may be old, refresh and retry
- lastRead = System.currentTimeMillis();
- if (notRacyClean(lastRead)) {
- return false;
- }
-
// We last read this path too close to its last observed
// modification time. We may have missed a modification.
// Scan again, to ensure we still see the same state.
On Thursday 03 February 2011 13:06:18 Dmitry Pavlenko wrote:
> Hello all,
> I've found a situation in which this patch doesn't work for me (Linux, Java
> 6):
>
> Here's the test code. "initialRepository" is mostly used for repository
> building, so it could be replaced by git command. The assertation
> "Assert.assertEquals(anotherCommitId1, anotherCommitId2);" is failed for
> me. I don't know the reason but I guess this is because of sth related to
> FileSnapshot#isModified checks. If I remove some "resolve" calls (that
> I've marked) or "sleep" call, the test is passed.
>
> Object "repository2" is independent from "repository1", so instead
> "repository1" there could be some external tool like native git runner. So
> this situation is from the real life.
>
> The test code
>
>
> final File gitDir = ...;
> final Repository initialRepository = new FileRepository(girDir);
> initialRepository.create(false);
>
> final DirCache dirCache = DirCache.newInCore();
> ObjectInserter objectInserter = initialRepository.newObjectInserter();
> final ObjectId treeId;
> try {
> treeId = dirCache.writeTree(objectInserter);
> }
> finally {
> objectInserter.release();
> }
>
> CommitBuilder commitBuilder;
>
> commitBuilder = new CommitBuilder();
> commitBuilder.setAuthor(new PersonIdent(initialRepository));
> commitBuilder.setCommitter(new PersonIdent(initialRepository));
> commitBuilder.setMessage("Initial commit");
> commitBuilder.setTreeId(treeId);
>
> objectInserter = initialRepository.newObjectInserter();
> final ObjectId commitId;
> try {
> commitId = objectInserter.insert(commitBuilder);
> }
> finally {
> objectInserter.release();
> }
>
> RefUpdate refUpdate;
>
> refUpdate = initialRepository.updateRef("refs/heads/master");
> refUpdate.setNewObjectId(commitId);
> refUpdate.forceUpdate();
>
> refUpdate = initialRepository.updateRef("HEAD");
> refUpdate.link("refs/heads/master");
>
> Repository repository1 = new FileRepository(gitDir);
> final ObjectId headId1 = repository1.resolve("HEAD"); //without this
> "resolve" the test is passed
>
> Repository repository2 = new FileRepository(gitDir);
> final ObjectId headId2 = repository2.resolve("HEAD"); //without this
> "resolve" the test is passed
>
> Assert.assertEquals(headId1, headId2);
> Assert.assertEquals(commitId, headId1);
>
> commitBuilder = new CommitBuilder();
> commitBuilder.setAuthor(new PersonIdent(initialRepository));
> commitBuilder.setCommitter(new PersonIdent(initialRepository));
> commitBuilder.setMessage("Another commit");
> commitBuilder.setParentId(commitId);
> commitBuilder.setTreeId(treeId);
>
> objectInserter = initialRepository.newObjectInserter();
> final ObjectId anotherCommitId;
> try {
> anotherCommitId = objectInserter.insert(commitBuilder);
> }
> finally {
> objectInserter.release();
> }
>
> refUpdate = repository1.updateRef("HEAD");
> refUpdate.setNewObjectId(anotherCommitId);
> refUpdate.forceUpdate();
>
> //we prepared two repository objects for the same repository directory
> //now let's resolve HEAD in both of them
>
> Thread.sleep(4000); //without this "sleep" the test is passed
>
> final ObjectId anotherCommitId2 = repository2.resolve("HEAD");
> final ObjectId anotherCommitId1 = repository1.resolve("HEAD");
>
> Assert.assertEquals(anotherCommitId1, anotherCommitId2);
> Assert.assertEquals(anotherCommitId1, anotherCommitId);
>
> On Tuesday 14 December 2010 17:42:43 Dmitry Pavlenko wrote:
> > Thank you, this helps.
> >
> > On Tuesday 14 December 2010 03:51:43 you wrote:
> > > On Sat, Dec 11, 2010 at 8:02 AM, Dmitry Pavlenko <dmit10@xxxxxxx> wrote:
> > > > I seems that FileBasedConfig (and loose-refs-related classes) relies
> > > > on java.io.File.lastModified() call (at least in isOutdated()
> > > > method).
> > >
> > > The following Gerrit changes try to fix this problem:
> > > http://egit.eclipse.org/r/2114
> > > http://egit.eclipse.org/r/2115
> > > http://egit.eclipse.org/r/2116
> > > http://egit.eclipse.org/r/2117
> > > http://egit.eclipse.org/r/2118
> > > http://egit.eclipse.org/r/2119
> > >
> > > Unfortunately its a bit too accurate. 2117 ("FileBasedConfig: Use
> > > FileSnapshot for isOutdated") shows an application programming bug
> > > with regards to Repository getConfig(). Once this change goes into
> > >
> > > JGit, applications cannot do:
> > > Repository db = ...;
> > > db.getConfig().setString(....);
> > > db.getConfig().save();
> > >
> > > Because db.getConfig() might re-read the file and discard the set that
> > > the application just performed. Yikes, that's one heck of a breakage.
>
> _______________________________________________
> jgit-dev mailing list
> jgit-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/jgit-dev
