From d13918310f54cd395dab0d1d0cb83c68b3900cd9 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Fri, 6 Apr 2018 14:05:37 -0600 Subject: [PATCH] Retry stale file handles on .git/config file On a local non-NFS filesystem the .git/config file will be orphaned if it is replaced by a new process while the current process is reading the old file. The current process successfully continues to read the orphaned file until it closes the file handle. Since NFS servers do not keep track of open files, instead of orphaning the old .git/config file, such a replacement on an NFS filesystem will instead cause the old file to be garbage collected (deleted). A stale file handle exception will be raised on NFS clients if the file is garbage collected (deleted) on the server while it is being read. Since we no longer have access to the old file in these cases, the previous code would just fail. However, in these cases, reopening the file and rereading it will succeed (since it will open the new replacement file). Since retrying the read is a viable strategy to deal with stale file handles on the .git/config file, implement such a strategy. Since it is possible that the .git/config file could be replaced again while rereading it, loop on stale file handle exceptions, up to 5 extra times, trying to read the .git/config file again, until we either read the new file, or find that the file no longer exists. The limit of 5 is arbitrary, and provides a safe upper bounds to prevent infinite loops consuming resources in a potential unforeseen persistent error condition. Change-Id: I6901157b9dfdbd3013360ebe3eb40af147a8c626 Signed-off-by: Nasser Grainawi Signed-off-by: Matthias Sohn --- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../jgit/storage/file/FileBasedConfig.java | 85 ++++++++++++------- 3 files changed, 56 insertions(+), 31 deletions(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index b5b04b474..c949c0aa9 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -123,6 +123,7 @@ commitMessageNotSpecified=commit message not specified commitOnRepoWithoutHEADCurrentlyNotSupported=Commit on repo without HEAD currently not supported commitAmendOnInitialNotPossible=Amending is not possible on initial commit. compressingObjects=Compressing objects +configHandleIsStale=config file handle is stale, {0}. retry connectionFailed=connection failed connectionTimeOut=Connection time out: {0} contextMustBeNonNegative=context must be >= 0 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 758f71d27..d32e873c1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -182,6 +182,7 @@ public class JGitText extends TranslationBundle { /***/ public String commitOnRepoWithoutHEADCurrentlyNotSupported; /***/ public String commitAmendOnInitialNotPossible; /***/ public String compressingObjects; + /***/ public String configHandleIsStale; /***/ public String connectionFailed; /***/ public String connectionTimeOut; /***/ public String contextMustBeNonNegative; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java index 4b4822c76..9a3884644 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java @@ -65,13 +65,19 @@ import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.FileUtils; import org.eclipse.jgit.util.IO; import org.eclipse.jgit.util.RawParseUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * The configuration file that is stored in the file of the file system. */ public class FileBasedConfig extends StoredConfig { + private final static Logger LOG = LoggerFactory + .getLogger(FileBasedConfig.class); + private final File configFile; private boolean utf8Bom; @@ -135,41 +141,58 @@ public class FileBasedConfig extends StoredConfig { */ @Override public void load() throws IOException, ConfigInvalidException { - final FileSnapshot oldSnapshot = snapshot; - final FileSnapshot newSnapshot = FileSnapshot.save(getFile()); - try { - final byte[] in = IO.readFully(getFile()); - final ObjectId newHash = hash(in); - if (hash.equals(newHash)) { - if (oldSnapshot.equals(newSnapshot)) - oldSnapshot.setClean(newSnapshot); - else - snapshot = newSnapshot; - } else { - final String decoded; - if (isUtf8(in)) { - decoded = RawParseUtils.decode(RawParseUtils.UTF8_CHARSET, - in, 3, in.length); - utf8Bom = true; + final int maxStaleRetries = 5; + int retries = 0; + while (true) { + final FileSnapshot oldSnapshot = snapshot; + final FileSnapshot newSnapshot = FileSnapshot.save(getFile()); + try { + final byte[] in = IO.readFully(getFile()); + final ObjectId newHash = hash(in); + if (hash.equals(newHash)) { + if (oldSnapshot.equals(newSnapshot)) { + oldSnapshot.setClean(newSnapshot); + } else { + snapshot = newSnapshot; + } } else { - decoded = RawParseUtils.decode(in); + final String decoded; + if (isUtf8(in)) { + decoded = RawParseUtils.decode( + RawParseUtils.UTF8_CHARSET, in, 3, in.length); + utf8Bom = true; + } else { + decoded = RawParseUtils.decode(in); + } + fromText(decoded); + snapshot = newSnapshot; + hash = newHash; } - fromText(decoded); + return; + } catch (FileNotFoundException noFile) { + if (configFile.exists()) { + throw noFile; + } + clear(); snapshot = newSnapshot; - hash = newHash; - } - } catch (FileNotFoundException noFile) { - if (configFile.exists()) { - throw noFile; + return; + } catch (IOException e) { + if (FileUtils.isStaleFileHandle(e) + && retries < maxStaleRetries) { + if (LOG.isDebugEnabled()) { + LOG.debug(MessageFormat.format( + JGitText.get().configHandleIsStale, + Integer.valueOf(retries)), e); + } + retries++; + continue; + } + throw new IOException(MessageFormat + .format(JGitText.get().cannotReadFile, getFile()), e); + } catch (ConfigInvalidException e) { + throw new ConfigInvalidException(MessageFormat + .format(JGitText.get().cannotReadFile, getFile()), e); } - clear(); - snapshot = newSnapshot; - } catch (IOException e) { - final IOException e2 = new IOException(MessageFormat.format(JGitText.get().cannotReadFile, getFile())); - e2.initCause(e); - throw e2; - } catch (ConfigInvalidException e) { - throw new ConfigInvalidException(MessageFormat.format(JGitText.get().cannotReadFile, getFile()), e); } }