Session should not throw java.util.ServiceConfigurationError

[jmehrens]

Running the test suite under JDK7 produces the following:

Testcase: testCloseContextClassLoader(com.sun.mail.util.logging.MailHandlerTest): Caused an ERROR javax.mail.Provider: Provider com.sun.mail.imap.IMAPProvider not found java.util.ServiceConfigurationError: javax.mail.Provider: Provider com.sun.mail.imap.IMAPProvider not found at java.util.ServiceLoader.fail(ServiceLoader.java:231) at java.util.ServiceLoader.access$300(ServiceLoader.java:181) at java.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:365) at java.util.ServiceLoader$1.next(ServiceLoader.java:445) at javax.mail.Session.loadProviders(Session.java:964) at javax.mail.Session.<init>(Session.java:254) at javax.mail.Session.getInstance(Session.java:281) at com.sun.mail.util.logging.MailHandler.initSession(MailHandler.java:3448) at com.sun.mail.util.logging.MailHandler.writeLogRecords0(MailHandler.java:2823) at com.sun.mail.util.logging.MailHandler.writeLogRecords(MailHandler.java:2790) at com.sun.mail.util.logging.MailHandler.close(MailHandler.java:853) at com.sun.mail.util.logging.MailHandlerTest.testCloseContextClassLoader0(MailHandlerTest.java:2295) at com.sun.mail.util.logging.MailHandlerTest.testCloseContextClassLoader(MailHandlerTest.java:2273)

This does not occur under JDK8 and later due to https://bugs.openjdk.java.net/browse/JDK-7198496

The session should:

1. Not let ServiceConfigurationError be thrown to the caller. Session was retrofitted to use service loader in GH 323 Support loading protocol providers using ServiceLoader and the behavior change of throwing was not added to the spec. Instead SCE should be logged to the debug logger and continue.
2. Providers that are not default providers should be loaded by context class loader then failback to the given classloader, then ClassLoader.getSystemClassLoader(), and finally ignore.
3. Providers that are default providers should use the classloader of the Provider.class then failback to the given cl.
4. The service loader iterator must run until fully exhausted. This will ensure that a single failure doesn't prevent other providers from being loaded.

https://github.com/eclipse-ee4j/mail/blob/31688b7e1860e83292ec32fb656808447fe13ccb/mail/src/main/java/javax/mail/Session.java#L976

[jmehrens]

Given the following test: Properties props = new Properties(); Session s = Session.getInstance(props); //assertEquals(6, s.getProviders().length); for(Provider p : s.getProviders()) { System.out.println(p); }

The providers are double the expected count. It appears that the META-INF/javamail.default.providers and the ServiceLoader are both loading the defaults without checking for duplicates. The duplicates should be harmless it just seems like an oversight.

[Airblader]

I'm running into an issue that would be fixed by these changes as well, I believe. In our system we end up loading Provider and IMAPProvider (and others) from different classloaders. This causes the SPI lookup to fail, which we wouldn't care about, but unfortunately Session doesn't recover from this and instead bails.

[jbescos]

I am looking into this but I have few questions.

If we don't fail with ServiceConfigurationError we will fail with a NullPointerException later, when the provider implementation is required. This will make the issue more difficult to spot by developers.

I suggest to move every Service#load inside a new static inner class inside Session. This will make that load lazily, so it will fail when it is required and it will show the details of the issue.

What is CCE?.

[Airblader]

If we don't fail with ServiceConfigurationError we will fail with a NullPointerException later, when the provider implementation is required

There's a bunch more ways to provide providers, though, SPI is just an additional one that happens to be tried first.

[jmehrens]

@jbescos CCE = context class loader. The default implementation ships with providers. If no providers load they are pragmatically added.. Where do we fail with NPE? We should fail with NoSuchProviderException when no providers are loaded and a caller has asked for them.

I had already coded up a solution but the issue is that when I started testing I noticed that this ticket is going to generate more issue tickets because there are more issues with how providers are loaded. I suspect there is an issue with how the internal provider structures are holding duplicate providers. The issue is that if two providers have the same name but are in different classloaders they are not duplicates per the getService method. If they have the same class loader and same name they are duplicates. I don't think this is an issue as getService perform lookups from different classloaders. I just want to make sure.

Once I have done that I can push may changes as PR for review and create new issues for everything else I find that is broken. Do you want me remove this issue from my queue and assign to you?