[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jgit-dev] JGit's overuse of IOException

I narrowed down the scope to JGit's server-side's UploadPack and ReceivePack and wrote a proposal on this.

https://docs.google.com/document/d/1QjdSKlOK_hOJrLkbgV43QGQH6QefJLvHx-TVrMEVL3k/edit?usp=sharing

There two proposals in this document:

1. Create ClientIOException and ServerIOException in the transport package.
2. Consolidate the UploadPack and ReceivePack error handling code.

I think 1 has almost no side effects. 2 needs to be done carefully so that the existing callers of UploadPack and ReceivePack are not affected.

--
Masaya Suzuki
draftcode@xxxxxxxxx


On Fri, Nov 16, 2018 at 5:23 AM Andrey Loskutov <loskutov@xxxxxx> wrote:
FYI: from Platform point of view, we have here org.eclipse.core.runtime.CoreException in Eclipse (which can't be used in JGit AFAIK).
There are few subclasses for it, but in the "usual" Platform code I'm very seldom see a use of subclasses. Most code just defines/checksÂCoreException.
Â
Kind regards,
Andrey Loskutov

ÐÐÐÑÐÐÐÐ ÑÑÐÐÐÑÑÐÑ - ÐÐÐÐ ÑÑÐ ÑÐÐÐÑ ÑÑÐÐÐÑÑÐÑ

http://google.com/+AndreyLoskutov
Â
Â
Gesendet:ÂDonnerstag, 15. November 2018 um 00:50 Uhr
Von:Â"Masaya Suzuki" <draftcode@xxxxxxxxx>
An:Â"Jonathan Nieder" <jrn@xxxxxxxxxx>
Cc:Â"Masaya Suzuki" <masayasuzuki@xxxxxxxxxx>, jgit-dev@xxxxxxxxxxx
Betreff:ÂRe: [jgit-dev] JGit's overuse of IOException
> A common superclass for JGit exceptions sounds harmless to me if JGit never uses it. :)
Â
Note that I want this common exception specifically for JGit's low-level parts. I think JGit client code (org.eclipse.jgit.api.*) and JGit server code (ReceivePack and UploadPack) might just want to catch with this root exception class since they are anyway not interested in what kind of JGitException it is in many cases.
Â
> The main other API I like to look at as an example, Guava, doesn't have a common base class for its exceptions. Is there a reason callers that don't care about the particular exception type would need such a thing instead of using Exception?
Â
When an interface is defined (the word "interface" here is a generic term to refer an abstraction), there should be a case where "as a user, I don't care which implementation it is, but I just want one". I think this root exception is also such interface. The question to be asked should be "is there a case where a caller wants to catch only exceptions thrown by JGit, but doesn't care if it's a CorruptPackIndexException or a DirCacheNameConflictException?". I think there is a case. To answer your question more directly, yes. A caller might not be interested in whether an exception thrown by JGit is a particular exception, but it doesn't mean they are not interested in any Exception type. For example, imagine that there is a server that wants to return a blob content for some request. There would be code like:
Â
  try (Repository repo = openRepository(...)) {
  ObjectReader or = repo.getObjectReader();
  if (!or.hasObjectId(requestedObjectId)) {
   throw new BadRequestException("%s doesn't exist in the repository", requestedObjectId);
  }
  return or.open(requestedObjectId).getBytes();
 } catch (JGitException e) {
  throw new BackendRuntimeException(e);
 }
Â
If there's no JGitException and the server needs to catch IndexReadException, CorruptPackIndexException, MissingObjectException, TooLargeObjectInPackException, UnsupportedPackVersionException, UnsupportedPackIndexVersionException, PackInvalidException, PackMismatchException, etc.. (I'm guessing this list from the names, but I don't think I'm that wrong.) I think forcing callers to list all of them in the catch clause would be impractical.
Â
If this code is rewritten to catch Exception:
Â
 ÂÂtry (Repository repo = openRepository(...)) {
  ObjectReader or = repo.getObjectReader();
  if (!or.hasObjectId(requestedObjectId)) {
   throw new BadRequestException("%s doesn't exist in the repository", requestedObjectId);
  }
  return or.open(requestedObjectId).getBytes();
 } catch (BadRequestException e) {
  // We need to re-catch this so that this is not converted to BackendRuntimeException.
  throw e;
 } catch (Exception e) {
  // Doubtful...
  throw new BackendRuntimeException(e);
 }
Because catch Exception will catch RuntimeExceptions, it needs to catch it inside the code to avoid wrapped to another exception. Having catch Exception makes a code smell. If this try-with-resources block starts using some library that is not from JGit (like making an RPC?) those exceptions are caught by this Exception catch, which was really meant for JGit exceptions only.
Â
Guava, and I think Apache commons also, are a collection of common utility classes. I guess even if Guava defines a root exception, there's no case that a caller want to catch any exception from Guava. There's no relationship in, say, ImmutableList and InetAddress, then there's no case a caller wants to catch a common "GuavaException".
Â
That said, I tried to find if there's an example, but I couldn't find a library that defines many checked exceptions. Java standard library defines a lot of checked exceptions, but I'm not sure if it's a good example... This is a Javadoc page that I hostÂhttps://docs.java-plus-you-download.today/javadoc/overview-tree.html, and it contains multiple OSS libraries. Looking at the Exception tree, I can see exceptions like GeneralSecurityException, SocketException, etc. that have a hierarchy, but not from the 3rd party ones. Also I cannot find an example that have a flat structure (i.e. all exceptions inherit directly from Exception).
Â
--
Masaya Suzuki
draftcode@xxxxxxxxx
Â
On Tue, Nov 13, 2018 at 8:24 PM Jonathan Nieder <jrn@xxxxxxxxxx> wrote:
Masaya Suzuki wrote:
Â
I think you probably meant to quote p252 "Item 62:ÂDocument all exceptions thrown by each method". It says "Don't take the shortcut of declaring that a method throws some superclass of multiple exception classes that it can throw". I think the exception can inherit from this root exception for the caller's convenience, but still can specify the individual exceptions for each method.Â
Â
I really meant item 61, but you make a good point. A common superclass for JGit exceptions sounds harmless to me if JGit never uses it. :)
Â
I suppose the clearest name for such a thing would be JGitException, since it would be used even by parts of the API, like config parsing, that don't involve a repository.
Â
The main other API I like to look at as an example, Guava, doesn't have a common base class for its exceptions. Is there a reason callers that don't care about the particular exception type would need such a thing instead of using Exception?
Â
Thanks,
Jonathan
Â
_______________________________________________ jgit-dev mailing list jgit-dev@xxxxxxxxxxx To change your delivery options, retrieve your password, or unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jgit-dev
_______________________________________________
jgit-dev mailing list
jgit-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://www.eclipse.org/mailman/listinfo/jgit-dev