8186738: (sl) ServiceLoader::stream doesn't update cache

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

8186738: (sl) ServiceLoader::stream doesn't update cache

Alan Bateman
There are are two issues in ServiceLoader that we crept in with re-write
here in JDK 9. One is with the new stream method where the Provider
elements are specified to be cached but the implementation dropped it in
the final version. The second is specific to the security manager
scenario where  errors encountered loading  or initializing providers (a
static initializer throws an error for example) aren't wrapped with the
ServiceConfigurationError.

The changes to address these issues are trivial and I'd like to get them
fixed in jdk10/master:
    http://cr.openjdk.java.net/~alanb/8186738/webrev/index.html

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: 8186738: (sl) ServiceLoader::stream doesn't update cache

Mandy Chung


On 10/12/17 2:11 PM, Alan Bateman wrote:

> There are are two issues in ServiceLoader that we crept in with
> re-write here in JDK 9. One is with the new stream method where the
> Provider elements are specified to be cached but the implementation
> dropped it in the final version. The second is specific to the
> security manager scenario where  errors encountered loading  or
> initializing providers (a static initializer throws an error for
> example) aren't wrapped with the ServiceConfigurationError.
>
> The changes to address these issues are trivial and I'd like to get
> them fixed in jdk10/master:
>    http://cr.openjdk.java.net/~alanb/8186738/webrev/index.html
>

Looks good in general.

test/jdk/java/util/ServiceLoader/security/test/p/Tests.java

184 assertTrue(e.getCause() instanceof Error);

I think it's more precise to check e.getCause().getClass() == Error.class.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: 8186738: (sl) ServiceLoader::stream doesn't update cache

Alan Bateman
On 12/10/2017 23:54, mandy chung wrote:
> :
>
> test/jdk/java/util/ServiceLoader/security/test/p/Tests.java
> 184 assertTrue(e.getCause() instanceof Error);
> I think it's more precise to check e.getCause().getClass() == Error.class.
>
You are right, that test should be improved, more so if each of cases
throws a different error type.

While testing this, I ran into an issue with the JRE and CLDR locale
providers adpaters - they silently swallow exceptions and so were
silently consuming security exceptions thrown by tests in in other areas
(logging and resource bundle tests mostly). I've created JDK-8189272 to
track this and in the mean-time, these two adpaters need to re-throw the
SCE when it's not for a security exception.

I've put the webrev with these changes here:
    http://cr.openjdk.java.net/~alanb/8189264/webrev/index.html

-Alan.

Reply | Threaded
Open this post in threaded view
|

Re: 8186738: (sl) ServiceLoader::stream doesn't update cache

Mandy Chung


On 10/13/17 6:51 AM, Alan Bateman wrote:

> On 12/10/2017 23:54, mandy chung wrote:
>> :
>>
>> test/jdk/java/util/ServiceLoader/security/test/p/Tests.java
>> 184 assertTrue(e.getCause() instanceof Error);
>> I think it's more precise to check e.getCause().getClass() ==
>> Error.class.
>>
> You are right, that test should be improved, more so if each of cases
> throws a different error type.
>

The test update looks good.

> While testing this, I ran into an issue with the JRE and CLDR locale
> providers adpaters - they silently swallow exceptions and so were
> silently consuming security exceptions thrown by tests in in other
> areas (logging and resource bundle tests mostly). I've created
> JDK-8189272 to track this and in the mean-time, these two adpaters
> need to re-throw the SCE when it's not for a security exception.
>
> I've put the webrev with these changes here:
> http://cr.openjdk.java.net/~alanb/8189264/webrev/index.html

+1

Throwing SCE if not caused for security is okay for this patch.   I
agree that if there is any problem loading these providers, it should be
made fatal and JDK-8189272 should consider that.

Mandy