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

[Valerie Peng]

On Wed, 12 Feb 2025 20:46:31 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 (fields)
>>         * `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 registering services through the Current or the Legacy API)
>>         * `boolean putService(Service svc)`
>>         * `boolean removeService(Service svc)`
>>         * `boolean putClassNameLegacy(ServiceKey key, String className, String propKey)`
>>         * `boolean putAliasLegacy(ServiceKey key, ServiceKey aliasKey, String propKey)`
>>         * `boolean putAttributeLegacy(ServiceKey key, String attrName, String attrValue, String propKey)`
>>         * `boolean removeLegacy(ServiceKey key, String className)`
>>         * `boolean removeAliasLegacy(ServiceKey key, ServiceKey aliasKey)`
>>         * `boolean removeAttributeLegacy(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 [GitHub Actions run](https://github.com/franferrax/jdk/actions/runs/12193211398))
>> * `jdk/com/sun/crypto`
>> * `jd...
>
> Francisco Ferrari Bihurriet has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Clear ServicesMap fields in the declared order
>   
>   Constructors assign the fields in the same order.

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

> 1484:      * case, Properties map entries are synchronized. In the latter, Properties
> 1485:      * map entries are not modified.
> 1486:      */

I am not too sure I get this comment block. Judging from the impl, this enum seems to be used to indicate whether the Properties map is updated. The part about ServiceMap is especially confusing. Is this enum for Properties map or ServicesMap? In addition, instead of stating "an entry that is not a service, alias or attribute.", can we just state the remaining types? Is "In the former case" refer to the UPDATE? In that paragraph. there is only one case. Lastly, there is only 2 values to this enum, can't this be replaced with a boolean?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1982747199