8345139: Fix bugs and inconsistencies in the Provider services map #22613

franferrax · 2024-12-06T19:36:09Z

Hi, this pull request implements the fixes for bugs and inconsistencies described in JDK-8345139.

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 ⁽¹⁾
  - serviceAttrProps (Map<ServiceKey, Map<UString, String>>): keeps track of the provider Hashtable entries that originated service attributes ⁽¹⁾
  - 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)
jdk/com/sun/crypto
jdk/java/security
- Including the new jdk/java/security/Provider/ServicesConsistency.java

Additionally, we found no regressions with respect to this pull request baseline (bf0debc) in the following category:

jdk/sun/security
- Same results in both codebases
  
  Results
  
  Tests that passed 797
  
  Tests that failed 15
  
  Tests that had errors 2
  
  Tests that were not run 35
  
  Total 849

New `ServicesConsistency.java` test

The new ServicesConsistency test checks several of the cases described in the JBS issue, plus other similar variants. Here is a mapping between some of the test cases and their corresponding JBS issue section (using text fragments links), known to describe the tested problem.

Test case(s)	JBS section
`ServicesConsistency::testInvalidGetServicesRemoval`	JDK-8345139, section 1.1
`ServicesConsistency::testInvalidGetServiceRemoval`	JDK-8345139, section 1.2
`ServicesConsistency::testPutAllIsAtomic` `ServicesConsistency::testReplaceAllIsAtomic`	JDK-8345139, section 1.4
`ServicesConsistency::testInvalidCachedClass` `ServicesConsistency::testInvalidCachedHasKeyAttributes` `ServicesConsistency::testInvalidCachedSupportedKeyFormats`	JDK-8345139, section 1.5
`ServicesConsistency::testSerializationClassnameConsistency`	JDK-8345139, section 2.1
`ServicesConsistency::testCurrentAPIServicesOverride`	JDK-8345139, section 2.3
`ServicesConsistency::testLegacyAPIServicesOverride`	JDK-8345139, section 2.4
`ServicesConsistency::testLegacyAPIAliasCannotBeAlgorithm`	JDK-8345139, section 2.5
`ServicesConsistency::testInvalidServiceNotReturned`	JDK-8345139, section 3.1 (and JDK-8344361)
`ServicesConsistency::testComputeDoesNotThrowNPE` `ServicesConsistency::testMergeDoesNotThrowNPE`	JDK-8345139, section 3.2

This contribution is co-authored by @franferrax and @martinuy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8345139: Fix bugs and inconsistencies in the Provider services map (Bug - P4)

Contributors

Francisco Ferrari Bihurriet <[email protected]>
Martin Balao <[email protected]>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22613/head:pull/22613
$ git checkout pull/22613

Update a local copy of the PR:
$ git checkout pull/22613
$ git pull https://git.openjdk.org/jdk.git pull/22613/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22613

View PR using the GUI difftool:
$ git pr show -t 22613

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22613.diff

Using Webrev

Link to Webrev Comment

Co-authored-by: Francisco Ferrari Bihurriet <[email protected]> Co-authored-by: Martin Balao <[email protected]>

franferrax · 2024-12-06T19:37:02Z

/contributor add fferrari

franferrax · 2024-12-06T19:37:12Z

/contributor add mbalao

bridgekeeper · 2024-12-06T19:42:50Z

👋 Welcome back fferrari! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-06T19:43:26Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-12-06T19:44:06Z

@franferrax
Contributor Francisco Ferrari Bihurriet <[email protected]> successfully added.

openjdk · 2024-12-06T19:44:36Z

@franferrax
Contributor Martin Balao <[email protected]> successfully added.

openjdk · 2024-12-06T19:45:30Z

@franferrax The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-12-06T19:49:15Z

Webrevs

seanjmullan · 2024-12-11T17:23:27Z

/reviewers 2 Reviewer

seanjmullan · 2024-12-11T17:23:59Z

This is a large change so I think it should require at least 2 Reviewer approvals.

openjdk · 2024-12-11T17:24:14Z

@seanjmullan
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

ascarpino · 2024-12-11T19:08:52Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * Enum to inform the result of an operation on the services map.
+         */
+        enum SvcOpResult {


Why use an enum here when a boolean is sufficient?

Just to add some more semantic value but we don't have a strong opinion, we can replace it with boolean if you want.

Yes, please do that.

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

bridgekeeper · 2025-01-09T07:58:02Z

@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ascarpino

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 are in ServiceMapImpl.

ascarpino · 2025-01-21T23:54:02Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * Enum to inform the result of an operation on the services map.
+         */
+        enum SvcOpResult {


Yes, please do that.

ascarpino · 2025-01-22T00:53:13Z

src/java.base/share/classes/java/security/Provider.java

+         */
+        private Service(Provider provider, ServiceKey algKey) {
+            assert algKey.algorithm.intern() == algKey.algorithm :
+                    "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.

These assertions are internal invariants that we want to sanity-check and document, specially for future code changes. They will never occur and do not depend on user API input. We went through all of them again to make sure that they are correct. For example, ServiceKey instances received in the referred Service constructor parameter must always have an internalized algorithm (the original algorithm string converted to uppercase). If you know of a call stack which may allow non-internalized algKey.algorithm, please let us know because it is a bug —we couldn't find any.

In the Security Manager days, the assumption was that Providers had enough privileges already to be trusted with internalizing Strings. On the contrary, code invoking getService was not necessarily trusted enough to internalize Strings used for queries. While this is not longer the case, we kept the distinction because Providers will use the Strings indeed and is a reduced set, whereas queries may be more open bounded.

ascarpino · 2025-01-23T01:19:45Z

src/java.base/share/classes/java/security/Provider.java

+                    "the algorithm.";
+            assert aliasKey.type.equals(type) : "Invalid alias key type.";
+            assert aliasKey.algorithm.intern() == aliasKey.algorithm :
+                    "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.

Please have a look at my comment above.

openjdk · 2025-02-11T20:18:56Z

@franferrax this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8345139
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

Fix conflict caused by e20bd01: 1. Ignore the change, as this had already been identified and fixed (see JDK-8345139, section 3.1). 2. Remove the test, as it is already covered by ServicesConsistency::testInvalidServiceNotReturned. 3. Add the corresponding bug ID to ServicesConsistency. Co-authored-by: Francisco Ferrari Bihurriet <[email protected]> Co-authored-by: Martin Balao Alonso <[email protected]>

martinuy · 2025-02-12T00:08:04Z

The reason for having Current and Legacy interfaces was only to internally group methods and stress the distinction. We have now removed it and proposed a flat hierarchy for the ServicesMap (without the interfaces and impl class).

franferrax · 2025-02-12T15:36:10Z

NOTE: as explained in the 35e2eae merge commit message, this PR now deletes InvalidServiceTest.java because that test is already covered by ServicesConsistency.java, since JDK-8344361 corresponds with JDK-8345139, section 3.1.

jdk/test/jdk/java/security/Provider/ServicesConsistency.java

Lines 835 to 839 in 35e2eae

    
           private static void testInvalidServiceNotReturned() throws Throwable { 
        
               p.put(aliasPropKeyL, algL); 
        
               assertServiceEqual(sT, algL, null); 
        
               assertServiceEqual(sT, aliasL, null); 
        
           }

Just in case, we made sure that the deleted InvalidServiceTest.java passes in this PR branch.

Constructors assign the fields in the same order.

valeriepeng · 2025-02-25T19:40:24Z

src/java.base/share/classes/java/security/Provider.java

+            classCache = null;
+            constructorCache = null;
+            hasKeyAttributes = null;
+            supportedFormats = null;
+            supportedClasses = null;


Are these necessary? The other constructor didn't set them.

We made these fields initialization explicit in the copy-constructor to document that cached fields need to be regenerated, and it's not that we forgot (or were added later). That's why we added the comment in the lines before.

valeriepeng · 2025-02-25T19:45:47Z

src/java.base/share/classes/java/security/Provider.java

+            } else {
+                attributes = new HashMap<>(svc.attributes);
+            }
+            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?

The value in this case could have been true as this constructor is only used when doing a copy-on-write from a service that is registered already in the map (the new/updated service will be in the map as well). We preferred to do this at the caller level. We can remove this explicit assignment from here.

valeriepeng · 2025-02-25T23:49:41Z

src/java.base/share/classes/java/security/Provider.java

-        void removeAttribute(String type, String value) {
-            if (attributes.isEmpty()) {
-                return;
-            }
-            if (value == null) {
-                attributes.remove(new UString(type));
-            } else {
-                attributes.remove(new UString(type), value);
-            }
+        private void removeAttribute(String attrName, String attrValue) {
+            UString attrKey = new UString(attrName);
+            assert attributes.get(attrKey) == attrValue :
+                    "Attribute value expected to exist with the same identity.";
+            attributes.remove(attrKey, attrValue);
        }


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.

Yes, the new implementation assumes attrValue isn't null.

For cases in which the old implementation was receiving null, the new implementation receives oldValue, from Provider::implRemove:

jdk/src/java.base/share/classes/java/security/Provider.java

Lines 1535 to 1539 in 3d0df51

private Object implRemove(Object key) {

Object oldValue = super.get(key);

return doLegacyOp(servicesMap, key, oldValue, null, OPType.REMOVE) ==

PropertiesMapAction.UPDATE ? super.remove(key) : oldValue;

}

This is also asserted as attrValue != null in Provider.ServicesMap::removeAttributeLegacy, the caller of Service::removeAttribute:

jdk/src/java.base/share/classes/java/security/Provider.java

Lines 1319 to 1329 in 3d0df51

assert key != null && attrName != null && attrValue != null :

"Attribute information missing.";

return updateSvc(key, (MappingInfo oldMi, Service newSvc) -> {

Map<UString, String> oldAttrProps =

serviceAttrProps.get(oldMi.algKey);

if (oldAttrProps != null) {

// The service was found and has attributes.

assert oldMi.svc != null :

"Inconsistent service attributes data.";

newSvc.removeAttribute(attrName, attrValue);

valeriepeng · 2025-02-26T00:07:43Z

src/java.base/share/classes/java/security/Provider.java

            this.className = className;
            if (aliases == null) {
                this.aliases = Collections.emptyList();
            } else {
                this.aliases = new ArrayList<>(aliases);
            }
+            aliasKeys = Collections.emptyMap();
            if (attributes == null) {
                this.attributes = Collections.emptyMap();
            } else {
                this.attributes = new HashMap<>();


Initialize with attributes.size() and load factor 1.0 if both are the same size?

valeriepeng · 2025-02-26T00:21:48Z

src/java.base/share/classes/java/security/Provider.java

+        // For services added to a ServicesMap, this is a map from alias service
+        // keys to alias string values. Empty map if no aliases. While map
+        // entries derive from the aliases field, keys are not repeated
+        // (case-insensitive comparison) and not equal to the algorithm. For
+        // 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.

We will re-write this comment.

valeriepeng · 2025-02-26T01:56:58Z

src/java.base/share/classes/java/security/Provider.java

+        // added to this map.
+        private final Map<ServiceKey, Map<UString, String>> serviceAttrProps;
+
+        ServicesMap() {


nit: add comment for this constructor?

Ok, I'll add a note.

valeriepeng · 2025-03-03T18:31:19Z

src/java.base/share/classes/java/security/Provider.java

@@ -635,7 +634,7 @@ public synchronized Object merge(Object key, Object value,

    // let javadoc show doc from superclass
    @Override
-    public Object get(Object key) {
+    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?

The reason why we need to synchronize get is because the handling of the properties map from ServicesMap may expose temporary holes. For example, if a property backed up by a service is overwritten with a value that is not (e.g. a non-string value), we need to delete the service first. When deleting the service, the properties map will have a temporary hole that is then filled up with the new property value.

It's possible that we missed some other methods such as getProperty. We will review them again.

valeriepeng · 2025-03-03T23:24:29Z

src/java.base/share/classes/java/security/Provider.java

+         * values.
+         */
+        private record MappingInfo(Service svc, ServiceKey algKey,
+                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?

isLegacy is null when the service was never added to the ServicesMap and the Current vs. Legacy API question does not apply. In these cases, the MappingInfo record returned by find has a null service too, indicating that the query did not find the service. For a caller, this case reads as the isLegacy value of a not found service, and will be ignored. You can check how all uses of <mapping-info>.isLegacy are in a conditional block where <mapping-info>.svc is not null or in a method where we have that pre-condition.

valeriepeng · 2025-03-04T01:55:06Z

src/java.base/share/classes/java/security/Provider.java

+                    } else {
+                        // The service was added with the Current API. Overwrite
+                        // the alias entry on the services map without modifying
+                        // 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"?

Yes, "associated with" is better. The overwrite happens later in putService. I'll clarify that in the comment.

valeriepeng · 2025-03-04T01:59:58Z

src/java.base/share/classes/java/security/Provider.java

+                return false;
+            }
+
+            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?

There is no check because entries added with the Legacy API can be removed (i.e. overwritten) with entries added with the Current API. Current API operations take precedence.

Looks like someone can invoke removeService with a Service instance whose algorithm was added with the Legacy API and the code is not stopping this removal. May be a good idea to stop this. @franferrax , what do you think?

We discussed this further with @franferrax and don't necessarily see the proposed behavior as a problem. To put this into some context first, it's very unlikely that someone builds a Service instance with the same algorithm of a Legacy API entry and invokes removeService. Even if that's the case, the service will be removed and the principle of "the Current API takes precedence" applies. This principle is also used when overriding a Legacy API value with putService —behavior that comes from before of our change. Our motivation for enforcing no modification of services added with the Current API from the Legacy API was to preserve service immutability, which would not be at stake here.

valeriepeng · 2025-03-05T01:02:15Z

src/java.base/share/classes/java/security/Provider.java

+                String canonicalPropKey = propKey;
+                if (oldMi.svc != null) {
+                    // The service exists. Get its Properties map entry. Note:
+                    // Services added through an alias or an attribute may don't


"may don't"? Maybe you mean "may not" or simply "don't"

Good catch. I'd go with "may not" because they have have been added without and later obtained one.

valeriepeng · 2025-03-05T01:40:55Z

src/java.base/share/classes/java/security/Provider.java

+        /*
+         * This method tries to find a service on the map (based on an
+         * algorithm or alias) and pass a copy of it to an update callback
+         * (copy-on-write). If the service found was added with the Current API,
+         * no update should be done. If a service was not found, a new instance
+         * may be created.
+         *
+         * The updated version of the service is put on the services map.
+         * Algorithm and alias based entries pointing to the old version of the
+         * service are overwritten.
+         */


Comment for this method should note that this method is only for the Legacy registration...

Sounds good. I added a note.

valeriepeng · 2025-03-05T07:06:45Z

src/java.base/share/classes/java/security/Provider.java

+        private boolean updateSvc(ServiceKey key,
+                ServiceUpdateCallback updateCb) {
+            Service newSvc;
+            MappingInfo oldMi = find(key);


oldMi could just simply be mi? There is no newMi in the same method anywhere.

We used old to stress the difference with the new service that is added to replace the old one. For example, in the statement newSvc = new Service(oldMi.svc) we see more clearly the contrast. There isn't a newMi because we don't look up in the map again. Unless you have a strong preference, I'd stick with old. Let us know.

valeriepeng · 2025-03-06T06:10:09Z

src/java.base/share/classes/java/security/Provider.java

+    /*
+     * Enum to determine if changes to the Properties map must be applied by the
+     * caller (UPDATE) or skipped (SKIP).
+     *
+     * If a change does not concern a ServicesMap, UPDATE is returned. An
+     * example of this is when adding, modifying or removing an entry that is
+     * not a service, alias or attribute.
+     *
+     * If the change concerns a ServicesMap, SKIP is returned. The change may
+     * have been applied internally or ignored due to an error. In the former
+     * case, Properties map entries are synchronized. In the latter, Properties
+     * map entries are not modified.
+     */


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?

Yes, we can replace a 2-values enum with a boolean if you have a strong preference. We have done this before. I still think that an enum and a comment may help the reader, especially because uses are in multiple private and undocumented methods. This enum allows us to have the documentation in a single place.

As for the comment itself, UPDATE indicates that the change in the outer (properties) map has to be done. SKIP indicates that the outer map change should be skipped: it was either done automatically when the ServicesMap was updated or should not be done because there was an error.

valeriepeng · 2025-03-06T06:13:22Z

src/java.base/share/classes/java/security/Provider.java

+            } else if (value != null && oldValue instanceof String oldValueS &&
+                    opType == OPType.ADD) {


Which method is this else-block for? value is not null and not instanceof String and oldValue is instanceof String and opType is ADD?

In this case we are adding an entry to the Provider (seen as a Properties map more than a Provider). The value of this new entry is not a string and is replacing an entry with the same key, whose value is mapped to a service in the internal (services) map. From the point of view of the internal map, this acts as a removal.

valeriepeng · 2025-03-06T06:15:17Z

src/java.base/share/classes/java/security/Provider.java

+                // From the ServicesMap point of view, this could be equivalent
+                // to a removal. In any case, let the caller proceed with the
+                // Properties map update.
+                parseLegacy(servicesMap, ks, oldValueS, OPType.REMOVE);


no return here and falls through to the line 1512? Is this really intended?

Yes, it's intended because we need the caller to handle the property change. While a service is removed in the internal (services) map (and the corresponding property in the outer map automatically removed), nothing is added to the outer map for the new property. We need the caller to handle that.

valeriepeng · 2025-03-06T07:06:08Z

src/java.base/share/classes/java/security/Provider.java

-        }
-        return o;
+    private Object implRemove(Object key) {
+        Object oldValue = super.get(key);


oldValue isn't really old value, is it? Naming it oldValue and pass it to doLegacyOp() as value is very confusing.

Yes, I see why it could be confusing. We named it old thinking from the returned value's point of view (when there is a removal, we have to return the old/previous value). Perhaps we should rename it to just value.

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

bridgekeeper · 2025-05-01T23:42:15Z

@franferrax This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2025-05-30T03:02:31Z

@franferrax This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8345139: Fix bugs and inconsistencies in the Provider services map

95364f8

Co-authored-by: Francisco Ferrari Bihurriet <[email protected]> Co-authored-by: Martin Balao <[email protected]>

openjdk bot added the rfr Pull request is ready for review label Dec 6, 2024

openjdk bot added the security [email protected] label Dec 6, 2024

franferrax mentioned this pull request Dec 6, 2024

8315487: Security Providers Filter #15539

Open

4 tasks

ascarpino reviewed Dec 12, 2024

View reviewed changes

Copyright update.

bda8971

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

ascarpino reviewed Jan 24, 2025

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 11, 2025

martinuy and others added 2 commits February 11, 2025 18:10

ServicesMapImpl class and Current, Legacy interfaces removed.

ac45d7c

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Feb 11, 2025

Clear ServicesMap fields in the declared order

3d0df51

Constructors assign the fields in the same order.

valeriepeng reviewed Mar 4, 2025

View reviewed changes

valeriepeng reviewed Mar 5, 2025

View reviewed changes

valeriepeng reviewed Mar 6, 2025

View reviewed changes

martinuy and others added 3 commits April 2, 2025 21:10

Assorted minor changes.

413cd7f

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

Minor doc change missing.

384f2a7

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

Sync of methods reading the properties map.

da693b1

Co-authored-by: Martin Balao Alonso <[email protected]> Co-authored-by: Francisco Ferrari Bihurriet <[email protected]>

bridgekeeper bot closed this May 30, 2025

Results
Tests that passed	797
Tests that failed	15
Tests that had errors	2
Tests that were not run	35
Total	849

	private Object implRemove(Object key) {
	Object oldValue = super.get(key);
	return doLegacyOp(servicesMap, key, oldValue, null, OPType.REMOVE) ==
	PropertiesMapAction.UPDATE ? super.remove(key) : oldValue;
	}

	assert key != null && attrName != null && attrValue != null :
	"Attribute information missing.";
	return updateSvc(key, (MappingInfo oldMi, Service newSvc) -> {
	Map<UString, String> oldAttrProps =
	serviceAttrProps.get(oldMi.algKey);
	if (oldAttrProps != null) {
	// The service was found and has attributes.
	assert oldMi.svc != null :
	"Inconsistent service attributes data.";

	newSvc.removeAttribute(attrName, attrValue);

		} else if (value != null && oldValue instanceof String oldValueS &&
		opType == OPType.ADD) {

8345139: Fix bugs and inconsistencies in the Provider services map #22613

8345139: Fix bugs and inconsistencies in the Provider services map #22613

Uh oh!

Conversation

franferrax commented Dec 6, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New services map design

Testing

New ServicesConsistency.java test

Progress

Issue

Contributors

Reviewing

Uh oh!

franferrax commented Dec 6, 2024

Uh oh!

franferrax commented Dec 6, 2024

Uh oh!

bridgekeeper bot commented Dec 6, 2024

Uh oh!

openjdk bot commented Dec 6, 2024

Uh oh!

openjdk bot commented Dec 6, 2024

Uh oh!

openjdk bot commented Dec 6, 2024

Uh oh!

openjdk bot commented Dec 6, 2024

Uh oh!

mlbridge bot commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

seanjmullan commented Dec 11, 2024

Uh oh!

seanjmullan commented Dec 11, 2024

Uh oh!

openjdk bot commented Dec 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jan 9, 2025

Uh oh!

ascarpino left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 11, 2025

Uh oh!

martinuy commented Feb 12, 2025

Uh oh!

franferrax commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franferrax commented Dec 6, 2024 •

edited by openjdk bot

Loading

New `ServicesConsistency.java` test

mlbridge bot commented Dec 6, 2024 •

edited

Loading

ascarpino left a comment •

edited

Loading