Commons Discovery
  1. Commons Discovery
  2. DISCOVERY-11

Service.providers Enumeration does not catch and discard UnsatisfiedLinkErrors and ExceptionInInitializerErrors

    Details

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

      Windows, Sun JDK 1.5.0.10

      Description

      The enumeration created by Service.providers does not catch UnsatisfiedLinkErrors and ExceptionInInitializerErrors. The former can arise, if a class contains a call to System.loadLibrary(String) in its static initializer, while the latter will be thrown, when a runtime exception is thrown from the static initializer. Service.providers should catch and ignore these and it should simply discard the class provoking the error as not available. As of version 0.4, Commons Discovery just forwards these errors.

        Activity

        Michael Rudolf created issue -
        Hide
        Simone Tripodi added a comment -

        wouldn't be better letting users know that there are problem on loading their classes, instead of simply swallowing exceptions and ignoring the class loading?
        I'm worried that the proposed solution could drive crazy someone...

        Show
        Simone Tripodi added a comment - wouldn't be better letting users know that there are problem on loading their classes, instead of simply swallowing exceptions and ignoring the class loading? I'm worried that the proposed solution could drive crazy someone...
        Hide
        Joerg Schaible added a comment -

        No, it will drive you crazy, if you know you have a working implementation on the class path, but another one is failing that is found first. Then you will simply have nothing. Example: Image IO works with the SPI mechanism. If a jar registers a service for some obscure image format, but does not bundle all dependencies, it would mean, that the application itself can no longer handle any image format at all.

        Show
        Joerg Schaible added a comment - No, it will drive you crazy, if you know you have a working implementation on the class path, but another one is failing that is found first. Then you will simply have nothing . Example: Image IO works with the SPI mechanism. If a jar registers a service for some obscure image format, but does not bundle all dependencies, it would mean, that the application itself can no longer handle any image format at all.
        Hide
        Michael Rudolf added a comment -

        You are right, of course the user needs to know about the errors. Therefore, they should surely be logged. However, without catching these errors a single faulty service provider can crash the whole system or at least prevent the application from using other service providers.

        Faults should generally not propagate until the user sees an error message, they should be contained to the smallest realm possible. In this case, the application should continue to work, except that the faulty service provider will not provide any service.

        An ideal solution would be to make the behavior configurable, so that programmers can decide what to do depending on the service. For example, it does not make sense for an application to continue operating normally if a crucial service cannot be provided. If there is more than one provider for a service, the application should continue and maybe display a warning about not being able to load a certain service provider.

        Show
        Michael Rudolf added a comment - You are right, of course the user needs to know about the errors. Therefore, they should surely be logged. However, without catching these errors a single faulty service provider can crash the whole system or at least prevent the application from using other service providers. Faults should generally not propagate until the user sees an error message, they should be contained to the smallest realm possible. In this case, the application should continue to work, except that the faulty service provider will not provide any service. An ideal solution would be to make the behavior configurable, so that programmers can decide what to do depending on the service. For example, it does not make sense for an application to continue operating normally if a crucial service cannot be provided. If there is more than one provider for a service, the application should continue and maybe display a warning about not being able to load a certain service provider.
        Hide
        Simone Tripodi added a comment -

        thanks for your feedbacks! so what about catching not just Exception but even Throwable? I mean, instead of

                    private S getNextClassInstance() {
                        while (services.hasNext()) {
                            ResourceClass<S> info = services.nextResourceClass();
                            try {
                                return spi.newInstance(info.loadClass());
                            } catch (Exception e) {
                                // ignore
                            }
                        }
                        return null;
                    }
        

        we will have

                    private S getNextClassInstance() {
                        while (services.hasNext()) {
                            ResourceClass<S> info = services.nextResourceClass();
                            try {
                                return spi.newInstance(info.loadClass());
                            } catch (Throwable t) {
                                // TODO logging the exception
                            }
                        }
                        return null;
                    }
        

        sounds more reasonable according to your suggestions?

        @Michael: make the behavior configurable sounds the better "nice to have", but I wouldn't add a new API in this maintenance release, does it sound good to you?

        Show
        Simone Tripodi added a comment - thanks for your feedbacks! so what about catching not just Exception but even Throwable? I mean, instead of private S getNextClassInstance() { while (services.hasNext()) { ResourceClass<S> info = services.nextResourceClass(); try { return spi.newInstance(info.loadClass()); } catch (Exception e) { // ignore } } return null ; } we will have private S getNextClassInstance() { while (services.hasNext()) { ResourceClass<S> info = services.nextResourceClass(); try { return spi.newInstance(info.loadClass()); } catch (Throwable t) { // TODO logging the exception } } return null ; } sounds more reasonable according to your suggestions? @Michael: make the behavior configurable sounds the better "nice to have", but I wouldn't add a new API in this maintenance release, does it sound good to you?
        Hide
        Joerg Schaible added a comment -

        No, Errors should not be catched in general. Here it is enough to catch additionally LinkageError that covers all the problems that arise from dynamical class loading including the ones mentioned explicitly by Michael.

        Show
        Joerg Schaible added a comment - No, Errors should not be catched in general. Here it is enough to catch additionally LinkageError that covers all the problems that arise from dynamical class loading including the ones mentioned explicitly by Michael.
        Hide
        Simone Tripodi added a comment -

        Fine, thanks for the feedback, I'm going to modify the code according to your suggestions

        I didn't want to hijack this thread, so I raised another issue related to this enumeration, can you help me please having a look at DISCOVERY-17?
        Many thanks in advance!

        Show
        Simone Tripodi added a comment - Fine, thanks for the feedback, I'm going to modify the code according to your suggestions I didn't want to hijack this thread, so I raised another issue related to this enumeration, can you help me please having a look at DISCOVERY-17 ? Many thanks in advance!
        Hide
        Simone Tripodi added a comment -

        Fixed according Joerg's suggestion, see Service r1090329

        Show
        Simone Tripodi added a comment - Fixed according Joerg's suggestion, see Service r1090329
        Simone Tripodi made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Simone Tripodi [ simone.tripodi ]
        Fix Version/s 0.5 [ 12312208 ]
        Resolution Fixed [ 1 ]
        Hide
        Sebb added a comment -

        BTW, some Throwables must always be propagated - for example ThreadDeath and VirtualMachineError.

        Show
        Sebb added a comment - BTW, some Throwables must always be propagated - for example ThreadDeath and VirtualMachineError.
        Hide
        Simone Tripodi added a comment -

        included in discovery-0.5 release

        Show
        Simone Tripodi added a comment - included in discovery-0.5 release
        Simone Tripodi made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development