11 Replies Latest reply on Sep 10, 2004 12:24 PM by norbert

    exception design

    visionlink

      i was wondering why all the TreeCache methods throw Exception instead of something more specific?

      for example, setIsolationLevel does:

       if(tmp_level == null) {
       throw new Exception("TreeCache.setIsolationLevel(): level \"" + level + "\" is invalid");
       }
      

      why not throw an InvalidArgumentException here?

      all the fun methods (put, get, remove) also throw Exception, it seems like this makes it much more difficult on the caller to know what to handle and when, etc.

      thanks.

        • 1. Re: exception design
          belaban

          You are absolutely right, changed 2 "new Exception" clauses to "new IllegalArgEx".

          The put()/get()/remove() etc methods have already been changed (recently). They now type their exceptions more specifically.

          It would be great if you could point me to some methods I have overlooked.

          Bela

          • 2. Re: exception design
            visionlink

            ok, great.

            i've been looking at the 3.2 cvs branch. should i go ahead and move to the head? is the head jboss-cache code compatible with the 3.2 server?

            • 3. Re: exception design
              belaban

              No, head is very different from 3.2. I will backport (a.k.a. 'copy') head to 3.2 in a few days though, this will then be part of 3.2.6 or 3.2.7.

              Bela

              • 4. Re: exception design
                norbert

                just noticed that all methods in TreeCache that make use of invokeMethod are designed to throw LockingException and TimeOutException but actually will allways throw NestedRuntimeException which is the only Exception being thrown by invokeMethod().

                as a result TreeCache behaves differently in respect to the exceptions being thrown if being used as a local Cache or within a group (which should be transparent to the programmer).

                The expected behavior can be archived by wrapping every call to invokeMethod with this:

                 try {
                 return (Node)invokeMethod(m);
                 } catch (NestedRuntimeException e) {
                 Throwable t = ((NestedRuntimeException)e).getNested();
                 if (t instanceof LockingException) throw (LockingException) t;
                 if (t instanceof TimeoutException) throw (TimeoutException) t;
                 if (t instanceof RuntimeException) throw (RuntimeException) t;
                 throw e;
                 }
                


                • 5. Re: exception design
                  belaban

                  Hmm, I'm not really happy about this either. I think I'm going to replace all typed exceptions in the signatures (e.g. LockingException, TimeoutException) by CacheException (the 2 are subclasses of CacheException), and have invokeMethod() throw CacheException (if it is one).

                  What do you think ?

                  Bela

                  • 6. Re: exception design
                    belaban

                    Okay, here's what I did:

                    - I replaced all "throws LockingException,TimeoutException" with "throws CacheException".

                    CacheException is a superclass of both exceptions.

                    - invokeMethod() throw CacheException as well now

                    This allows us to (a) throw additional methods in the future, e.g. SecurityException (extends CacheException) when we add a SecurityInterceptor and (b) does *not* break existing code. E.g.

                    try {
                    cache.put("fqn", "key", "val");
                    }
                    catch(LockingException lex) {}
                    catch(TimeoutException tex) {}

                    will still work.

                    Bela

                    • 7. Re: exception design
                      norbert

                      I agree, this makes more sense and leaves room for future expansions, but it seems to be not completely sufficient.

                      There is the problem that a remote method-call may allways result in a number responses that are either not received or are exceptions themself. And just picking the first invalid response and throw TimeoutException or the Exception being found (whichever comes first) hides every other failure.

                      e.G.: there might be a member in the group that has some deployment or classloader issue, or runs out of memory which all results in runtimeexceptions. This should be visible for the sending member if synchronous replication is mandatory.

                      What about extending RuntimeException (like NestedRuntimeException) to hold a List of Throwables and throw this in case there is any unexpected Exception in the List of responses? The List would contain all Throwables found in the list of responses.

                      • 8. Re: exception design
                        visionlink

                         

                        "bela" wrote:
                        I think I'm going to replace all typed exceptions in the signatures (e.g. LockingException, TimeoutException) by CacheException (the 2 are subclasses of CacheException)


                        the unfortunate thing about this sort of design is that the caller now doesn't really know what to expect from the method being called. that is, it says that it throws CacheException, but the caller has no idea if the called method is capable of throwing a LockingException. it seems, to me, that being more specific in the throws clause is better from a caller's standpoint.

                        • 9. Re: exception design
                          norbert

                          I agree.

                          It's a good idea to make LockingException und TimeoutException subclasses of a CachingException, but it would be better if the method's signatures explicitly list the exceptions that actually may be thrown.

                          The caller would still be free to just use a 'catch (CacheException ..)' instead of being more specific.

                          The way it's designed now forces the caller to do so - existing code a la:

                          try {
                           put(...)
                          } catch (LockingException lex) {
                          } catch (TimeoutException tex) {
                          }
                          


                          does not work anymore (because the compiler knows there could be more subclasses to CacheException being added and thrown without being catched at any later time...)

                          • 10. Re: exception design
                            belaban

                            The problem with these strongly typed exceptions is that various interceptors that are going to be added in the future (or customer-supplied ones) might throw other exceptions, e.g. my example SecurityInterceptor throws a SecurityException (*not* a runtime exception).
                            This would have to be reflected in the interface as well, a.k.a. another change.
                            Strongly typed exceptions prevent this.
                            Suggestion: you could write a wrapper for JBossCache, that types CacheException into the various subtypes.

                            Bela

                            • 11. Re: exception design
                              norbert

                              I know about this - It's a decision whether to stay downwards compatible or change an API for reasons of better design that helps to stay downwards-compatible later...

                              Any way - if we keep it this way (and there are good reasons for this) we must put in the release-notes that this change might break existing code when upgrading from 1.02 to 1.1.