On Wed, 12 Feb 2025 20:46:31 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 (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 637: > 635: // let javadoc show doc from superclass > 636: @Override > 637: public synchronized Object get(Object key) { How about the getProperty(String) method on line 675? Add `@Override` tag and `synchronized` keyword there also? And the `keySet()` and `values()` methods on line 432 and 444 respectively? What is the criteria for synchronizing the method of the `Provider` class? src/java.base/share/classes/java/security/Provider.java line 713: > 711: */ > 712: private record MappingInfo(Service svc, ServiceKey algKey, > 713: Boolean isLegacy) {} There is comment states that `isLegacy` may be null, but then I also see a few if-cond using `isLegacy` directly like its a boolean, wouldn't it lead to NPE if `isLegacy` is `null`? src/java.base/share/classes/java/security/Provider.java line 735: > 733: // with the Legacy API. The absence of a service key on this set > is an > 734: // indication that the service was either not added or added > with the > 735: // Current API. Only algorithm service keys are added to this > set. nit: I find the sentence "The absence of a service key on this set ... added with Current API" is somewhat redundant. I suppose you mean "not added with the Legacy API or added with the Current API". The first sentence is clear enough and the second sentence doesn't add much value. src/java.base/share/classes/java/security/Provider.java line 750: > 748: private final Map<ServiceKey, Map<UString, String>> > serviceAttrProps; > 749: > 750: ServicesMap() { nit: add comment for this constructor? src/java.base/share/classes/java/security/Provider.java line 988: > 986: // The service was added with the Current API. > Overwrite > 987: // the alias entry on the services map without > modifying > 988: // the service that is currently using it. Is the "service" in the above line really means the provider `service` entry? If so, may be "associated with" is better than "using". Also there is no code under this comment block, where is the action of "overwrite the alias entry on the services map"? src/java.base/share/classes/java/security/Provider.java line 1037: > 1035: } > 1036: > 1037: if (mi.isLegacy) { For legacy entry, there is no check on the `legacyApiCall` value, is this due to the call path from `resolveKeyConflict` method? However, should a legacy entry be removed by the `removeService` method? If not, then additional check may be needed? src/java.base/share/classes/java/security/Provider.java line 1654: > 1652: } > 1653: > 1654: // used as key in the serviceMap and legacyMap HashMaps This comment is obsolete with the new impl and should be updated. src/java.base/share/classes/java/security/Provider.java line 2071: > 2069: // For services added to a ServicesMap, their algorithm service > key. > 2070: // This value derives from the algorithm field. For services > (still) > 2071: // not added to a ServicesMap, value is null. The current comment is a bit hard to read. How about "The mapping key of this service when added to a `ServiceMap`; null if not yet added to a `ServiceMap`"? The value is derived from the type and algorithm and this is straightforward enough that probably need not be commented. src/java.base/share/classes/java/security/Provider.java line 2078: > 2076: // entries derive from the aliases field, keys are not repeated > 2077: // (case-insensitive comparison) and not equal to the > algorithm. For > 2078: // services (still) not added to a ServicesMap, value is an > empty map. Could we re-write it to summarize the conditions for empty map? It could be easier to read/understand. For example: empty map if no aliases or if this service is not yet added to a `ServiceMap`. The part of case-insensitive comparision, it's due to the impl of `ServiceKey`, right? Maybe we can simply refer to that no need to describe it here. src/java.base/share/classes/java/security/Provider.java line 2110: > 2108: > 2109: /* > 2110: * Constructor used from the ServicesMap Legacy API. nit: "used **by**"? Same goes for other places. src/java.base/share/classes/java/security/Provider.java line 2149: > 2147: attributes = new HashMap<>(svc.attributes); > 2148: } > 2149: registered = false; I didn't see it's set to `true` in any of the constructors; also the default value is already `false`, why only explicitly set it to `false` here? src/java.base/share/classes/java/security/Provider.java line 2158: > 2156: hasKeyAttributes = null; > 2157: supportedFormats = null; > 2158: supportedClasses = null; Are these necessary? The other constructor didn't set them. src/java.base/share/classes/java/security/Provider.java line 2202: > 2200: "Attribute value expected to exist with the same > identity."; > 2201: attributes.remove(attrKey, attrValue); > 2202: } Is the new impl assuming `attrValue` should never be `null`? Based on javadoc of `Map.remove(Object key, Object value)`, the new impl removes the entry when the associated value is `null` vs the original impl removes the entry regardless of the associated value. src/java.base/share/classes/java/security/Provider.java line 2241: > 2239: this.attributes = Collections.emptyMap(); > 2240: } else { > 2241: this.attributes = new HashMap<>(); Initialize with `attributes.size()` and load factor 1.0 if both are the same size? src/java.base/share/classes/java/security/Provider.java line 2256: > 2254: * keys with Service::addAliasKey. > 2255: */ > 2256: private void generateServiceKeys() { nit: move this private method to be with other private methods? src/java.base/share/classes/java/security/Provider.java line 2268: > 2266: } > 2267: } > 2268: aliasKeys = Collections.unmodifiableMap(aliasKeys); if `aliases` are empty, then we can skip line 2261-2268 and no need to update `aliasKeys`? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1977987224 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1978378764 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970776267 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970784237 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1978485726 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1978488877 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970753720 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970723732 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970720272 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970724166 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970437584 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970428109 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970698200 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970710298 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970740763 PR Review Comment: https://git.openjdk.org/jdk/pull/22613#discussion_r1970736998
