On Wed, 8 Jan 2025 16:32:46 GMT, Francisco Ferrari Bihurriet <fferr...@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 <mba...@redhat.com> > Co-authored-by: Francisco Ferrari Bihurriet <fferr...@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
