Re: [jgit-dev] JGit removes Pack from list *even* if exists on the files

[Luca Milanesio <luca.milanesio@xxxxxxxxx>]

Hi Matthias, thanks for your fast reply.

See my feedback below.

On 10 Mar 2017, at 09:05, Matthias Sohn <matthias.sohn@xxxxxxxxx> wrote:

+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.

Yes, we could in theory but we shouldn't for sake of future-proof compatibility IMHO.

We do know that the file exists already, because of the previous if condition.

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.

I was thinking about that yesterday. We should have a maximum retry count logic.

This means that if JGit tried to open a pack file for N consecutive times without success and the pack file exists, it should then just give up and remove it from the list as it does today.

This parameter should be configurable with a "common sense" default value.

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

https://dev.eclipse.org/site_login/myaccount.php#open_tab_cla

and then push this change to Gerrit. 

We are not allowed to accept patches from users who haven't signed their ECA

I did already, and if I try to do it now it just says that it is done.

However, I still cannot push to Gerrit :-(

How long does it take for the Eclipse guys to approve my ECA ?

Luca.

-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.