Re: [jgit-dev] JGit's overuse of IOException

[Matthias Sohn <matthias.sohn@xxxxxxxxx>]

On Mon, May 21, 2018 at 9:00 PM, Jonathan Nieder <jrn@xxxxxxxxxx> wrote:

Hi,

Context: https://git.eclipse.org/r/c/119969/

Early JGit code seems to be a little confused about how to handle I/O errors. For example:

public Map<String, Ref> getAllRefs() {

try {

return getRefDatabase().getRefs(RefDatabase.ALL);

} catch (IOException e) {

return new HashMap<>();

}

}

This is dangerous and flaky. I'd like to deprecate any interfaces that do this kind of thing and vow to never do it again.

I agree

 

That opens a question of what methods like getAllRefs should throw. Our current answer has been to throw IOException and make all *other* ways the function can fail also inherit from IOException: https://imgur.com/a/k8yYPaP

This makes IOException behave like a cross between Exception and RuntimeException. Like RuntimeException, callers can expect a large part of JGit's API to throw it without declaring which subclasses they throw. Like Exception, the only way to handle it is to log and bail out, since the exception type does not indicate what actually happened in an actionable way. Methods can throw new subclasses of IOException without changing their method signature and callers are not likely to recover from them. This is unusual for a Java project and defeats the purpose of how Java checked exceptions are designed to work.

making many exceptions inherit from IOException stems from the very old days of jgit

For the higher level API in org.eclipse.jgit.api we added GitApiException and JGitInternalException

trying to find a balance between using checked exceptions deriving from GitApiException for errors

which clients are likely to handle and using RuntimeExceptions deriving from JGitInternalException

for lower level problems.

Having a common base class for JGit checked exceptions allows us to introduce new exceptions

in minor releases which would otherwise break the API. Not sure if this cheating is a good idea.

It's a compromise between breaking API frequently and using non-appropriate exceptions.

At the moment JGit defines 96 exception types which is a lot to handle for those who use JGit. 

Some stats for JGit exception classes

41 exceptions derive from IOException

34 exceptions derive from GitAPIException

Here some stats from EGit's usage of JGit exceptions:

  36 catch clauses catching some specific JGit exception

  48 catch GitAPIException clauses

  15 catch JGitInternalException clauses

398 catch IOException clauses (this includes IOExceptions not thrown by JGit)

 

Effective Java 2nd edition item 61 "Throw exceptions appropriate to the abstraction" has more advice on this subject. It's likely that in a some places where we currently throw IOException, we should be wrapping in a higher-level custom exception like ObjectAccessException. Alternatively, we can let IOException bubble through for low-level I/O errors and use a higher-level custom exception for higher-level errors like incorrect object type.

in the API package we convert from low level exceptions to either subclass of GitAPIException or JGitInternalException,
Though I am not sure if this is complete and consistent.

And there are many other packages which are also API.

 

Possible actions:

 1. Deprecate methods that swallow and hide I/O errors and adapt callers to use replacements for them. I've wished for this for a long time; sorry I didn't act on it for so long.

I also feel guilty. We could try to identify these bad cases asap and deprecate them already in 5.0

 

 2. Come up with a new proposed exception hierarchy for JGit exceptions and a migration path to using it. Would this be worth the migration pain? Does anyone have experience with making such changes in other libraries?

 

Hard to say without a concrete proposal for a better exception hierarchy.

 

Other thoughts?

Thanks,

Jonathan