RFR: 8345139: Fix bugs and inconsistencies in the Provider services map [v2]

[Anthony Scarpino]

On Wed, 8 Jan 2025 16:32:46 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:

>> Hi, this pull request implements the fixes for bugs and inconsistencies described in [JDK-8345139](https://bugs.openjdk.org/browse/JDK-8345139 "Fix bugs and inconsistencies in the Provider services map").
>> 
>> #### New services map design
>> 
>> Here is the high-level hierarchy of the new services map design:
>> 
>> * `servicesMap` (`ServicesMap`)
>>     * Instances
>>         * `impl` (`ServicesMapImpl`)
>>             * `services` (`Map<ServiceKey, Service>`): unifies the previous `serviceMap` and `legacyMap`
>>             * `legacySvcKeys` (`Set<ServiceKey>`): set indicating which keys in `services` correspond to the Legacy API
>>             * `serviceProps` (`Map<ServiceKey, String>`): keeps track of the _provider Hashtable entries_ that originated services entries <sup>(1)</sup>
>>             * `serviceAttrProps` (`Map<ServiceKey, Map<UString, String>>`): keeps track of the _provider Hashtable entries_ that originated service attributes <sup>(1)</sup>
>>         * `serviceSet` (`AtomicReference<Set<Service>>`): part of a lock-free mechanism to implement a consistent version of the `getServices()` readers method
>>     * Writers' methods (for providers registration)
>>         * `Current asCurrent()`: returns `impl` seen as a `Current` interface implementer
>>             * `putService(Service svc)`
>>             * `removeService(Service svc)`
>>         * `Legacy asLegacy()`: returns `impl` seen as a `Legacy` interface implementer
>>             * `putClassName(ServiceKey key, String className, String propKey)`
>>             * `putAlias(ServiceKey key, ServiceKey aliasKey, String propKey)`
>>             * `putAttribute(ServiceKey key, String attrName, String attrValue, String propKey)`
>>             * `remove(ServiceKey key, String className)`
>>             * `removeAlias(ServiceKey key, ServiceKey aliasKey)`
>>             * `removeAttribute(ServiceKey key, String attrName, String attrValue)`
>>     * Readers' methods (for services users and `GetInstance` APIs)
>>         * `Set<Service> getServices()`
>>         * `Service getService(ServiceKey key)`
>>     * Other methods: default and copy constructors, `void clear()`
>> 
>> (1): to maintain the consistency between the provider's `servicesMap` and its _Hashtable entries_, even if Legacy API updates occur through _properties_ with different casing, or aliases instead of main algorithms.
>> 
>> #### Testing
>> 
>> As part of our testing, we observed all the tests pass in the following categories:
>> 
>> * `jdk:tier1` (see [...
>
> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Copyright update.
>   
>   Co-authored-by: Martin Balao Alonso <mbalao at redhat.com>
>   Co-authored-by: Francisco Ferrari Bihurriet <fferrari at redhat.com>

Could you explain the need for `Current` and `Legacy` interfaces?   You have calls to  `doLegacy()` for adding and removing entries,  but I do not see why this is necessary since both APIs `ServiceMapImpl`.

src/java.base/share/classes/java/security/Provider.java line 2223:

> 2221:         private Service(Provider provider, ServiceKey algKey) {
> 2222:             assert algKey.algorithm.intern() == algKey.algorithm :
> 2223:                     "Algorithm should be interned.";

Why is `intern()` a requirement for this constructor?

Following the call stack this AssertionError is thrown with `Provider.load()` and `Provider.putAll()`  at a minimum.   This could change behavior and I think it should be removed.

src/java.base/share/classes/java/security/Provider.java line 2279:

> 2277:             assert aliasKey.type.equals(type) : "Invalid alias key type.";
> 2278:             assert aliasKey.algorithm.intern() == aliasKey.algorithm :
> 2279:                     "Alias should be interned.";

All these asserts look like they leak into the public API.  If something does not match your requirements, then log a detailed message using `debug` and do not add the entry.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22613#pullrequestreview-2565915562
PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1924543400
PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1926224994