Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] JGit's overuse of IOException 
On 21.05.2018 23:31, Matthias Sohn wrote:
On Mon, May 21, 2018 at 9:00 PM, Jonathan Nieder <jrn@xxxxxxxxxx <mailto:jrn@xxxxxxxxxx>> wrote: 

    Hi,

    Context: https://git.eclipse.org/r/c/119969/
    <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 <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.
The criterion "which clients are likely to handle" doesn't seem right to me. Also, letting the client crash for "unlikely" errors is no good choice from the user's perspective. It won't give him any details about the problem but just the impression that the client is unstable/buggy.
A better criterion would be "which can (theoretically) be handled by the user (not the client)". In this way, every error related to I/O should result in checked Exceptions: a file corruption may even be caused intentionally (by the user). Screwed up config files give a good example here.
On the other hand, e.g. invalid config files built in memory by bad usage of the JGit API should better be RuntimeExceptions and resulting in a crash of the application.
For low-level methods which don't know their context (e.g. whether being called with an in-memory Stream or a FileInputStream), better throw checked Exceptions. This will force the user of this method to think about whether the Exception case is actually possible for him. If not, the user may still turn it into some RuntimeException. 

-Marc

Back to the top