]> source.dussan.org Git - jgit.git/commitdiff
Retry loose object read upon "Stale file handle" exception 95/181295/12
authorAntonio Barone <syntonyze@gmail.com>
Wed, 2 Jun 2021 15:13:17 +0000 (18:13 +0300)
committerMatthias Sohn <matthias.sohn@sap.com>
Thu, 24 Jun 2021 21:52:22 +0000 (23:52 +0200)
When reading loose objects over NFS it is possible that the OS syscall
would fail with ESTALE errors: This happens when the open file
descriptor no longer refers to a valid file.

Notoriously it is possible to hit this scenario when git data is shared
among multiple clients, for example by multiple gerrit instances in HA.

If one of the two clients performs a GC operation that would cause the
packing and then the pruning of loose objects, the other client might
still hold a reference to those objects, which would cause an exception
to bubble up the stack.

The Linux NFS FAQ[1] (at point A.10), suggests that the proper way to
handle such ESTALE scenarios is to:

"[...] close the file or directory where the error occurred, and reopen
it so the NFS client can resolve the pathname again and retrieve the new
file handle."

In case of a stale file handle exception, we now attempt to read the
loose object again (up to 5 times), until we either succeed or encounter
a FileNotFoundException, in which case the search can continue to
Packfiles and alternates.

The limit of 5 provides an arbitrary upper bounds that is consistent to
the one chosen when handling stale file handles for packed-refs
files (see [2] for context).

[1] http://nfs.sourceforge.net/
[2] https://git.eclipse.org/r/c/jgit/jgit/+/54350

Bug: 573791
Change-Id: I9950002f772bbd8afeb9c6108391923be9d0ef51

org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/ObjectDirectoryTest.java
org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties
org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java

index d269457eb1f5a19a36a0c4f6b6537e421f8e8a20..a0bc63a69895931173460b722ac09eb59bd4d6f1 100644 (file)
@@ -44,8 +44,12 @@ package org.eclipse.jgit.internal.storage.file;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 import java.io.File;
 import java.io.IOException;
@@ -69,6 +73,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig;
 import org.eclipse.jgit.util.FS;
 import org.junit.Assume;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 public class ObjectDirectoryTest extends RepositoryTestCase {
 
@@ -194,6 +199,42 @@ public class ObjectDirectoryTest extends RepositoryTestCase {
                assertTrue(shallowCommits.isEmpty());
        }
 
+       @Test
+       public void testOpenLooseObjectSuppressStaleFileHandleException()
+                       throws Exception {
+               ObjectId id = ObjectId
+                               .fromString("873fb8d667d05436d728c52b1d7a09528e6eb59b");
+               WindowCursor curs = new WindowCursor(db.getObjectDatabase());
+
+               ObjectDirectory mock = mock(ObjectDirectory.class);
+               UnpackedObjectCache unpackedObjectCacheMock = mock(
+                               UnpackedObjectCache.class);
+
+               Mockito.when(mock.getObjectLoader(any(), any(), any()))
+                               .thenThrow(new IOException("Stale File Handle"));
+               Mockito.when(mock.openLooseObject(curs, id)).thenCallRealMethod();
+               Mockito.when(mock.unpackedObjectCache())
+                               .thenReturn(unpackedObjectCacheMock);
+
+               assertNull(mock.openLooseObject(curs, id));
+               verify(unpackedObjectCacheMock).remove(id);
+       }
+
+       @Test
+       public void testOpenLooseObjectPropagatesIOExceptions() throws Exception {
+               ObjectId id = ObjectId
+                               .fromString("873fb8d667d05436d728c52b1d7a09528e6eb59b");
+               WindowCursor curs = new WindowCursor(db.getObjectDatabase());
+
+               ObjectDirectory mock = mock(ObjectDirectory.class);
+
+               Mockito.when(mock.getObjectLoader(any(), any(), any()))
+                               .thenThrow(new IOException("some IO failure"));
+               Mockito.when(mock.openLooseObject(curs, id)).thenCallRealMethod();
+
+               assertThrows(IOException.class, () -> mock.openLooseObject(curs, id));
+       }
+
        @Test
        public void testShallowFileCorrupt() throws Exception {
                FileRepository repository = createBareRepository();
index c5e7dc3efa57d9c39d0cf50dcb466647ec3adaf8..2f3520de503334d30394e13217cda1b2d75b7d04 100644 (file)
@@ -416,6 +416,7 @@ logInconsistentFiletimeDiff={}: inconsistent duration from file timestamps on {}
 logLargerFiletimeDiff={}: inconsistent duration from file timestamps on {}, {}: diff = {} > {} (last good value). Aborting measurement.
 logSmallerFiletime={}: got smaller file timestamp on {}, {}: {} < {}. Aborting measurement at resolution {}.
 logXDGConfigHomeInvalid=Environment variable XDG_CONFIG_HOME contains an invalid path {}
+looseObjectHandleIsStale=loose-object {0} file handle is stale. retry {1} of {2}
 maxCountMustBeNonNegative=max count must be >= 0
 mergeConflictOnNonNoteEntries=Merge conflict on non-note entries: base = {0}, ours = {1}, theirs = {2}
 mergeConflictOnNotes=Merge conflict on note {0}. base = {1}, ours = {2}, theirs = {2}
index 814811d131327238b93ecdf94992058b6d01df4f..22200c26bcbffffd99ac5631239155600416c1c8 100644 (file)
@@ -444,6 +444,7 @@ public class JGitText extends TranslationBundle {
        /***/ public String logLargerFiletimeDiff;
        /***/ public String logSmallerFiletime;
        /***/ public String logXDGConfigHomeInvalid;
+       /***/ public String looseObjectHandleIsStale;
        /***/ public String maxCountMustBeNonNegative;
        /***/ public String mergeConflictOnNonNoteEntries;
        /***/ public String mergeConflictOnNotes;
index 265b71dd2a04182efa3b2b9b6f3fef1a0beb5fbd..9d1d4573692f57679cb60dd2b7f707ae7e23bfed 100644 (file)
@@ -85,6 +85,10 @@ public class ObjectDirectory extends FileObjectDatabase {
        /** Maximum number of candidates offered as resolutions of abbreviation. */
        private static final int RESOLVE_ABBREV_LIMIT = 256;
 
+       /** Maximum number of attempts to read a loose object for which a stale file
+        *  handle exception is thrown */
+       final static int MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS = 5;
+
        private final AlternateHandle handle = new AlternateHandle(this);
 
        private final Config config;
@@ -212,7 +216,7 @@ public class ObjectDirectory extends FileObjectDatabase {
        /** {@inheritDoc} */
        @Override
        public void close() {
-               unpackedObjectCache.clear();
+               unpackedObjectCache().clear();
 
                final PackList packs = packList.get();
                if (packs != NO_PACKS && packList.compareAndSet(packs, NO_PACKS)) {
@@ -277,7 +281,7 @@ public class ObjectDirectory extends FileObjectDatabase {
        /** {@inheritDoc} */
        @Override
        public boolean has(AnyObjectId objectId) {
-               return unpackedObjectCache.isUnpacked(objectId)
+               return unpackedObjectCache().isUnpacked(objectId)
                                || hasPackedInSelfOrAlternate(objectId, null)
                                || hasLooseInSelfOrAlternate(objectId, null);
        }
@@ -395,7 +399,7 @@ public class ObjectDirectory extends FileObjectDatabase {
        @Override
        ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId)
                        throws IOException {
-               if (unpackedObjectCache.isUnpacked(objectId)) {
+               if (unpackedObjectCache().isUnpacked(objectId)) {
                        ObjectLoader ldr = openLooseObject(curs, objectId);
                        if (ldr != null) {
                                return ldr;
@@ -473,23 +477,71 @@ public class ObjectDirectory extends FileObjectDatabase {
        @Override
        ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id)
                        throws IOException {
-               File path = fileFor(id);
+               int readAttempts = 0;
+               while (readAttempts < MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS) {
+                       readAttempts++;
+                       File path = fileFor(id);
+                       try {
+                               return getObjectLoader(curs, path, id);
+                       } catch (FileNotFoundException noFile) {
+                               if (path.exists()) {
+                                       throw noFile;
+                               }
+                               break;
+                       } catch (IOException e) {
+                               if (!FileUtils.isStaleFileHandleInCausalChain(e)) {
+                                       throw e;
+                               }
+                               if (LOG.isDebugEnabled()) {
+                                       LOG.debug(MessageFormat.format(
+                                                       JGitText.get().looseObjectHandleIsStale, id.name(),
+                                                       Integer.valueOf(readAttempts), Integer.valueOf(
+                                                                       MAX_LOOSE_OBJECT_STALE_READ_ATTEMPTS)));
+                               }
+                       }
+               }
+               unpackedObjectCache().remove(id);
+               return null;
+       }
+
+       /**
+        * Provides a loader for an objectId
+        *
+        * @param curs
+        *            cursor on the database
+        * @param path
+        *            the path of the loose object
+        * @param id
+        *            the object id
+        * @return a loader for the loose file object
+        * @throws IOException
+        *             when file does not exist or it could not be opened
+        */
+       ObjectLoader getObjectLoader(WindowCursor curs, File path, AnyObjectId id)
+                       throws IOException {
                try (FileInputStream in = new FileInputStream(path)) {
-                       unpackedObjectCache.add(id);
+                       unpackedObjectCache().add(id);
                        return UnpackedObject.open(in, path, id, curs);
-               } catch (FileNotFoundException noFile) {
-                       if (path.exists()) {
-                               throw noFile;
-                       }
-                       unpackedObjectCache.remove(id);
-                       return null;
                }
        }
 
+       /**
+        * <p>
+        * Getter for the field <code>unpackedObjectCache</code>.
+        * </p>
+        * This accessor is particularly useful to allow mocking of this class for
+        * testing purposes.
+        *
+        * @return the cache of the objects currently unpacked.
+        */
+       UnpackedObjectCache unpackedObjectCache() {
+               return unpackedObjectCache;
+       }
+
        @Override
        long getObjectSize(WindowCursor curs, AnyObjectId id)
                        throws IOException {
-               if (unpackedObjectCache.isUnpacked(id)) {
+               if (unpackedObjectCache().isUnpacked(id)) {
                        long len = getLooseObjectSize(curs, id);
                        if (0 <= len) {
                                return len;
@@ -567,13 +619,13 @@ public class ObjectDirectory extends FileObjectDatabase {
                        throws IOException {
                File f = fileFor(id);
                try (FileInputStream in = new FileInputStream(f)) {
-                       unpackedObjectCache.add(id);
+                       unpackedObjectCache().add(id);
                        return UnpackedObject.getSize(in, id, curs);
                } catch (FileNotFoundException noFile) {
                        if (f.exists()) {
                                throw noFile;
                        }
-                       unpackedObjectCache.remove(id);
+                       unpackedObjectCache().remove(id);
                        return -1;
                }
        }
@@ -667,7 +719,7 @@ public class ObjectDirectory extends FileObjectDatabase {
                        boolean createDuplicate) throws IOException {
                // If the object is already in the repository, remove temporary file.
                //
-               if (unpackedObjectCache.isUnpacked(id)) {
+               if (unpackedObjectCache().isUnpacked(id)) {
                        FileUtils.delete(tmp, FileUtils.RETRY);
                        return InsertLooseObjectResult.EXISTS_LOOSE;
                }
@@ -723,7 +775,7 @@ public class ObjectDirectory extends FileObjectDatabase {
                Files.move(FileUtils.toPath(tmp), FileUtils.toPath(dst),
                                StandardCopyOption.ATOMIC_MOVE);
                dst.setReadOnly();
-               unpackedObjectCache.add(id);
+               unpackedObjectCache().add(id);
                return InsertLooseObjectResult.INSERTED;
        }