Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] JGit removes Pack from list *even* if exists on the filesystem

+1, I think you are right, we should not remove packs if the pack file exists but we can't access it due to another reason.

We could try to further differentiate the causes for FileNotFoundException, by e.g. analysing the error message.

We should also think about how to prevent flooding the logs if such an error condition doesn't go away so that we keep logging
the same problem over and over again. This could have the effect that a single pack facing such a problem might stall
a JGit based server by excessive logging. Maybe we should quarantine affected packs and retry reading them
later after an increasing delay.

Please sign the ECA here
and then push this change to Gerrit. 
We are not allowed to accept patches from users who haven't signed their ECA

-Matthias

On Fri, Mar 10, 2017 at 1:34 AM, lucamilanesio <luca.milanesio@xxxxxxxxx> wrote:
Now the right diff (previous one was the negative):

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
index 1aa71e5..e7308b5 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
@@ -569,6 +569,7 @@ void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 
        private void handlePackError(IOException e, PackFile p) {
                String warnTmpl = null;
+               String errTmpl = JGitText.get().exceptionWhileReadingPack;
                if ((e instanceof CorruptObjectException)
                                || (e instanceof PackInvalidException)) {
                        warnTmpl = JGitText.get().corruptPack;
@@ -576,11 +577,11 @@ private void handlePackError(IOException e, PackFile p) {
                        removePack(p);
                } else if (e instanceof FileNotFoundException) {
                        if (p.getPackFile().exists()) {
-                               warnTmpl = JGitText.get().packInaccessible;
+                               errTmpl = JGitText.get().packInaccessible;
                        } else {
                                warnTmpl = JGitText.get().packWasDeleted;
+                               removePack(p);
                        }
-                       removePack(p);
                } else if (FileUtils.isStaleFileHandleInCausalChain(e)) {
                        warnTmpl = JGitText.get().packHandleIsStale;
                        removePack(p);
@@ -597,8 +598,7 @@ private void handlePackError(IOException e, PackFile p) {
                        // Don't remove the pack from the list, as the error may be
                        // transient.
                        LOG.error(MessageFormat.format(
-                                       JGitText.get().exceptionWhileReadingPack, p.getPackFile()
-                                                       .getAbsolutePath()), e);
+                                       errTmpl, p.getPackFile().getAbsolutePath()), e);
                }
        }

On Friday, March 10, 2017 at 12:32:09 AM UTC, lucamilanesio wrote:
Actually the JGit wiki is broken ... can't sign the ECA and post the fix :-(

Anyway, here is the patch:

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
index e7308b5..1aa71e5 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
@@ -569,7 +569,6 @@ void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 
        private void handlePackError(IOException e, PackFile p) {
                String warnTmpl = null;
-               String errTmpl = JGitText.get().exceptionWhileReadingPack;
                if ((e instanceof CorruptObjectException)
                                || (e instanceof PackInvalidException)) {
                        warnTmpl = JGitText.get().corruptPack;
@@ -577,11 +576,11 @@ private void handlePackError(IOException e, PackFile p) {
                        removePack(p);
                } else if (e instanceof FileNotFoundException) {
                        if (p.getPackFile().exists()) {
-                               errTmpl = JGitText.get().packInaccessible;
+                               warnTmpl = JGitText.get().packInaccessible;
                        } else {
                                warnTmpl = JGitText.get().packWasDeleted;
-                               removePack(p);
                        }
+                       removePack(p);
                } else if (FileUtils.isStaleFileHandleInCausalChain(e)) {
                        warnTmpl = JGitText.get().packHandleIsStale;
                        removePack(p);
@@ -598,7 +597,8 @@ private void handlePackError(IOException e, PackFile p) {
                        // Don't remove the pack from the list, as the error may be
                        // transient.
                        LOG.error(MessageFormat.format(
-                                       errTmpl, p.getPackFile().getAbsolutePath()), e);
+                                       JGitText.get().exceptionWhileReadingPack, p.getPackFile()
+                                                       .getAbsolutePath()), e);
                }
        }



On Friday, March 10, 2017 at 12:12:53 AM UTC, lucamilanesio wrote:
Hi Gerrit and JGit users,
I have found a small but serious bug in the way that JGit manages the file-system related errors when reading pack files.

Look at this broken management of the FileNotFoundException (from https://goo.gl/oidSwf)


The FileNotFoundException is typically raised in three conditions:
- file doesn't exist
- incompatible read vs. read/write open modes
- filesystem locking
- temporary lack of resources (e.g. too many open files)

... so ... *why on earth* do we always assume that Pack doesn't exist and we remove it from the in-memory list?

The "removePack(p)" should be IMHO inside the else() block whilst if FileNotFoundException happens when the pack file exists => log the error and try next time again.
Does it make sense?

Going to post a 1-liner fix tomorrow :-)

Luca.


Back to the top