Abdera
  1. Abdera
  2. ABDERA-279

Faulty classloading - unreachable code

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.1.1
    • Fix Version/s: None
    • Labels:
      None

      Description

      The Discover class uses this method to get a class:

            private static <T> Class<T> getClass(ClassLoader loader, String spec) {
              Class<T> c = null;
              try {
                  c = (Class<T>)loader.loadClass(spec);
                  c = (Class<T>)(c != null ? c : getClass(Discover.class.getClassLoader(), spec));
              } catch (ClassNotFoundException e) {
                  throw new RuntimeException(e);
              }
              return c;
          }
      

      However the first line inside the try statement will throw a CNF exception if the class cannot be found. As a result, the second line is never executed.

      c = (Class<T>)(c != null ? c : getClass(Discover.class.getClassLoader(), spec));
      

      The code executes recursively, which makes things harder. I would recommend changing the way the code is currently written to avoid recursion.

      1. fix_classloader.patch
        1.0 kB
        Antoine Toulme

        Activity

        Hide
        ant elder added a comment -

        Thanks Antoine, that does look wrong. Any chance you would provide a fix patch?

        Show
        ant elder added a comment - Thanks Antoine, that does look wrong. Any chance you would provide a fix patch?
        Hide
        Antoine Toulme added a comment -

        Can you delay 1.1.2 till I provide one ? It should be a matter of an hour but I can't contribute it before tonight/tomorrow night timeframe.

        Show
        Antoine Toulme added a comment - Can you delay 1.1.2 till I provide one ? It should be a matter of an hour but I can't contribute it before tonight/tomorrow night timeframe.
        Hide
        Antoine Toulme added a comment -

        Patch attached, I ran the tests and they all passed.

        Show
        Antoine Toulme added a comment - Patch attached, I ran the tests and they all passed.
        Hide
        Christine Koppelt added a comment -

        Would be great if you could also add some tests for your patch.

        Show
        Christine Koppelt added a comment - Would be great if you could also add some tests for your patch.
        Hide
        Antoine Toulme added a comment -

        Christine, where should I place those tests ? Where should I start ? It doesn't like there are tests for this class at all and the method is private static, which makes it difficult to test.

        Show
        Antoine Toulme added a comment - Christine, where should I place those tests ? Where should I start ? It doesn't like there are tests for this class at all and the method is private static, which makes it difficult to test.
        Hide
        ant elder added a comment -

        Patch applied, thanks for the fix Antoine.

        Tests would be good but as the class has no other testing already it seems ok to not require them for others.

        I needed to get the 1.1.2 release out for a fix for another user and as it was already being voted on it would have delayed it to long to include this. I can do a 1.1.3 release to include this fix now if you need it ASAP.

        Show
        ant elder added a comment - Patch applied, thanks for the fix Antoine. Tests would be good but as the class has no other testing already it seems ok to not require them for others. I needed to get the 1.1.2 release out for a fix for another user and as it was already being voted on it would have delayed it to long to include this. I can do a 1.1.3 release to include this fix now if you need it ASAP.
        Hide
        Antoine Toulme added a comment -

        Thanks! I already patched my way out of this on my side, so no need for a release I think.

        Show
        Antoine Toulme added a comment - Thanks! I already patched my way out of this on my side, so no need for a release I think.

          People

          • Assignee:
            Unassigned
            Reporter:
            Antoine Toulme
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development