Commons Discovery
  1. Commons Discovery
  2. DISCOVERY-17

Enumeration returned by Service.providers has a broken behavior

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 0.5
    • Fix Version/s: 0.5
    • Labels:
      None

      Description

      I find the current Enumeration behavior broken, but please tell me if I'm wrong!!!
      This is the actual current behavior:

      return new Enumeration<S>() {
                  private S object = getNextClassInstance();
      
                  public boolean hasMoreElements() {
                      return object != null;
                  }
      
                  public S nextElement() {
                      if (object == null) {
                          throw new NoSuchElementException();
                      }
      
                      S obj = object;
                      object = getNextClassInstance();
                      return obj;
                  }
      
                  private S getNextClassInstance() {
                      while (services.hasNext()) {
                          ResourceClass<S> info = services.nextResourceClass();
                          try {
                              return spi.newInstance(info.loadClass());
                          } catch (Exception e) {
                              // ignore
                          } catch (UnsatisfiedLinkError ule) {
                              // ignore
                          } catch (ExceptionInInitializerError eiie) {
                              // ignore
                          }
                      }
                      return null;
                  }
              };
      

      but it should be

      return new Enumeration<S>() {
      
                  public boolean hasMoreElements() {
                      return services.hasNext();
                  }
      
                  public S nextElement() {
                      ResourceClass<S> info = services.nextResourceClass();
                      try {
                          return spi.newInstance(info.loadClass());
                      } catch (Exception e) {
                          // ignore
                      } catch (UnsatisfiedLinkError ule) {
                          // ignore
                      } catch (ExceptionInInitializerError eiie) {
                          // ignore
                      }
                      return null;
                  }
              };
      

      I think there's no need to comment both codes, please tell me why the first one should be right

        Activity

        Hide
        Simone Tripodi added a comment - - edited

        Or, much better:

        return new Enumeration<S>() {
        
                    public boolean hasMoreElements() {
                        return services.hasNext();
                    }
        
                    public S nextElement() {
                        ResourceClass<S> info = services.nextResourceClass();
        
                        if (info == null) {
                            throw new NoSuchElementException();
                        }
        
                        try {
                            return spi.newInstance(info.loadClass());
                        } catch (Exception e) {
                            // ignore
                        } catch (UnsatisfiedLinkError ule) {
                            // ignore
                        } catch (ExceptionInInitializerError eiie) {
                            // ignore
                        }
                        return null;
                    }
                };
        
        Show
        Simone Tripodi added a comment - - edited Or, much better: return new Enumeration<S>() { public boolean hasMoreElements() { return services.hasNext(); } public S nextElement() { ResourceClass<S> info = services.nextResourceClass(); if (info == null ) { throw new NoSuchElementException(); } try { return spi.newInstance(info.loadClass()); } catch (Exception e) { // ignore } catch (UnsatisfiedLinkError ule) { // ignore } catch (ExceptionInInitializerError eiie) { // ignore } return null ; } };
        Hide
        Joerg Schaible added a comment -

        Hi Simone, as already said in DISCOVERY-11, you should catch LinkageError instead of those two individual ones. When dealing with dynamic class loading, you can get all of those, not only these two.

        Show
        Joerg Schaible added a comment - Hi Simone, as already said in DISCOVERY-11 , you should catch LinkageError instead of those two individual ones. When dealing with dynamic class loading, you can get all of those, not only these two.
        Hide
        Simone Tripodi added a comment -

        Hi Joerg,
        thanks for the reminder, I should have overlooked the LinkageError, unfortunately I didn't have the chance to working with it before (I'm relatively "young" )

        What about the proposed patch here? Does it make sense or it is just me having the feeling that the current implementation is wrong? In the worst case it always return the element on top services...
        Thanks in advance!

        Show
        Simone Tripodi added a comment - Hi Joerg, thanks for the reminder, I should have overlooked the LinkageError, unfortunately I didn't have the chance to working with it before (I'm relatively "young" ) What about the proposed patch here? Does it make sense or it is just me having the feeling that the current implementation is wrong? In the worst case it always return the element on top services ... Thanks in advance!
        Hide
        Joerg Schaible added a comment -

        BTW: A nice test scenario is to create service entries for all commons-logging implementations and iterate over them. Typically you don't have the Avalon LogKit in the class path.

        Show
        Joerg Schaible added a comment - BTW: A nice test scenario is to create service entries for all commons-logging implementations and iterate over them. Typically you don't have the Avalon LogKit in the class path.
        Hide
        Joerg Schaible added a comment - - edited

        Add it (= the attached file) to src/test/resources/META-INF/services.

        Show
        Joerg Schaible added a comment - - edited Add it (= the attached file) to src/test/resources/META-INF/services.
        Hide
        Simone Tripodi added a comment -

        Thanks for your help and hints Joerg, I'm playing with this silly test:

        @Test
        public void findImplementationsViaService() {
            Enumeration<? extends Log> logImplementations = Service.providers(Log.class);
        
            while (logImplementations.hasMoreElements()) {
                Log log = logImplementations.nextElement();
                System.out.println(log);
            }
        }
        

        what is print is just

        org.apache.commons.logging.impl.NoOpLog@1b016632
        

        switching to the proposed implementation instead, behavior looks like more consistent:

        null
        null
        org.apache.commons.logging.impl.NoOpLog@78dc6a77
        

        Jdk14Logger and LogKitLogger need a constructor argument to be built; indeed, modifying the "test" in the following way:

        @Test
        public void findImplementationsViaService() {
            Enumeration<? extends Log> logImplementations = Service.providers(new SPInterface<Log>(Log.class,
                    new Class<?>[]{ String.class },
                    new Object[]{ getClass().getName() }),
                    null);
        
            while (logImplementations.hasMoreElements()) {
                Log log = logImplementations.nextElement();
                System.out.println(log);
            }
        }
        

        I got the following output:

        org.apache.commons.logging.impl.Jdk14Logger@d8d9850
        org.apache.commons.logging.impl.LogKitLogger@502cb49d
        org.apache.commons.logging.impl.NoOpLog@70cb6009
        

        I would say that there are no more doubts at that point, current implementation is broken, WDYT? Thanks in advance!

        Show
        Simone Tripodi added a comment - Thanks for your help and hints Joerg, I'm playing with this silly test: @Test public void findImplementationsViaService() { Enumeration<? extends Log> logImplementations = Service.providers(Log.class); while (logImplementations.hasMoreElements()) { Log log = logImplementations.nextElement(); System .out.println(log); } } what is print is just org.apache.commons.logging.impl.NoOpLog@1b016632 switching to the proposed implementation instead, behavior looks like more consistent: null null org.apache.commons.logging.impl.NoOpLog@78dc6a77 Jdk14Logger and LogKitLogger need a constructor argument to be built; indeed, modifying the "test" in the following way: @Test public void findImplementationsViaService() { Enumeration<? extends Log> logImplementations = Service.providers( new SPInterface<Log>(Log.class, new Class <?>[]{ String .class }, new Object []{ getClass().getName() }), null ); while (logImplementations.hasMoreElements()) { Log log = logImplementations.nextElement(); System .out.println(log); } } I got the following output: org.apache.commons.logging.impl.Jdk14Logger@d8d9850 org.apache.commons.logging.impl.LogKitLogger@502cb49d org.apache.commons.logging.impl.NoOpLog@70cb6009 I would say that there are no more doubts at that point, current implementation is broken, WDYT? Thanks in advance!
        Hide
        Simone Tripodi added a comment -

        fixed on trunk, please review Service r1090660

        Show
        Simone Tripodi added a comment - fixed on trunk, please review Service r1090660
        Hide
        Joerg Schaible added a comment -

        Hi Simone,

        for me returning null is strange, but I am biased because I use a different ServiceLoader implementation that simply does behave in the former way. Can someone else give an opinion?

        BTW: The idea of the test for the log implementations is, that you do not have the avalon logger in your dependencies. The test should define and ensure the ServiceLoader's behavior in such a case.

        The original spec by Sun: http://download.oracle.com/javase/1.3/docs/guide/jar/jar.html#Service%20Provider

        Interesting is the look at javadocs of java.util.ServiceLoader.iterator() of Java 6 and javax.imageio.spi.ServiceRegistry.lookupProviders(Class) of Java 5.

        Show
        Joerg Schaible added a comment - Hi Simone, for me returning null is strange, but I am biased because I use a different ServiceLoader implementation that simply does behave in the former way. Can someone else give an opinion? BTW: The idea of the test for the log implementations is, that you do not have the avalon logger in your dependencies. The test should define and ensure the ServiceLoader's behavior in such a case. The original spec by Sun: http://download.oracle.com/javase/1.3/docs/guide/jar/jar.html#Service%20Provider Interesting is the look at javadocs of java.util.ServiceLoader.iterator() of Java 6 and javax.imageio.spi.ServiceRegistry.lookupProviders(Class) of Java 5.
        Hide
        Simone Tripodi added a comment -

        you're right Joerg, after reading the doc I'm convinced that the commit related to this issue has to be rolled back.
        thanks for reviewing!

        Show
        Simone Tripodi added a comment - you're right Joerg, after reading the doc I'm convinced that the commit related to this issue has to be rolled back. thanks for reviewing!
        Hide
        Simone Tripodi added a comment -

        commit has to be rolled back

        Show
        Simone Tripodi added a comment - commit has to be rolled back
        Hide
        Simone Tripodi added a comment -

        Rolled back the applied fix, the right behavior is the one reported in the Service Provider documentation and not the proposed one, thanks Joerg for reviewing

        Show
        Simone Tripodi added a comment - Rolled back the applied fix, the right behavior is the one reported in the Service Provider documentation and not the proposed one, thanks Joerg for reviewing
        Hide
        Simone Tripodi added a comment -

        included in discovery-0.5 release

        Show
        Simone Tripodi added a comment - included in discovery-0.5 release

          People

          • Assignee:
            Simone Tripodi
            Reporter:
            Simone Tripodi
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 20m
              20m
              Remaining:
              Remaining Estimate - 20m
              20m
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development