Why Eureka Client used Synchronized Map for Instance Metadata

[lsc1943]

We are using eureka client 1.x, and recently we found some threads were locked by "java.util.Collections$SynchronizedMap" which was happened while getting data from instanceInfo metadata under a high workload. It seems impact the performance. For the instance metadata, it's returned as a concurrentHashmap from server, but after deserialization, it turned to be a SynchronizedMap.

The below code is under EurekaJacksonCodec class, any reason why we are using synchronizedMap here?
builder.setMetadata(metadataMap == null ? Collections.emptyMap() : Collections.synchronizedMap(metadataMap));

[troshko111]

We are using eureka client 1.x, and recently we found some threads were locked by "java.util.Collections$SynchronizedMap" which was happened while getting data from instanceInfo metadata under a high workload. It seems impact the performance.

While possible, I do wonder under which scenarios you'd get high contention accessing individual instance metadata in the client? A flame graph would help if you have one handy.

For the instance metadata, it's returned as a concurrentHashmap from server, but after deserialization, it turned to be a SynchronizedMap.

Well it's returned as a map, that's the only contract. I don't think there's an expectation of any sort of type information to be preserved (beyond primitive, array and object) via JSON/XML across different languages, or even within Java.

The below code is under EurekaJacksonCodec class, any reason why we are using synchronizedMap here?
builder.setMetadata(metadataMap == null ? Collections.emptyMap() : Collections.synchronizedMap(metadataMap));

It's hard to say now, but I'd guess for simplicity's sake, I see no harm swapping this for any other thread safe implementation, if we can show a clear benefit.

[lsc1943]

@troshko111 , thank you for your comment. Our scenario is a little tricky, we have a gateway and hundreds of instances for an upstream service, once a request came in, the gateway will try to filter the instances by iterate the instanceInfo list and get metatdata for each instanceInfo, and when the concurrent workload is high, we saw the locked instances of "java.util.Collections$SynchronizedMap" from JFR as many concurrent requests may try to get the metadata at the same time for a single instanceInfo.
For the (builder.setMetadata) here, why do we need a thread safe implementation here, under any condition the eureka client will try to build or modify the metadata concurrently?
Thank you!

[troshko111]

This eureka-core/src/main/java/com/netflix/eureka/resources/InstanceResource.java:237 uses ConcurrentHashMap as well, but AFAICT this is a long deprecated API as it's effectively equivalent to re-registering (which is what it does internally).

I went over other usages, and see no explicit contract demanding a synchronized map, in fact xstream serializer uses a HashMap.

My conclusion is that this code is more likely to have a bug than not:

                    case METADATA:
                        Map<String, String> metadataMap = null;
                        while ((jsonToken = jp.nextToken()) != JsonToken.END_OBJECT) {
                            char[] parserChars = jp.getTextCharacters();
                            if (parserChars[0] == '@' && EnumLookup.equals(BUF_AT_CLASS, parserChars, jp.getTextOffset(), jp.getTextLength())) {
                                // skip this
                                jsonToken = jp.nextToken();
                            }
                            else { // For backwards compatibility
                                String key = intern.apply(jp, CacheScope.GLOBAL_SCOPE);
                                jsonToken = jp.nextToken();
                                String value = intern.apply(jp, CacheScope.APPLICATION_SCOPE );
                                metadataMap = Optional.ofNullable(metadataMap).orElseGet(METADATA_MAP_SUPPLIER);
                                metadataMap.put(key, value);
                            }
                        };   
                        builder.setMetadata(metadataMap == null ? Collections.emptyMap() : Collections.synchronizedMap(metadataMap));
                        break;

Looks to me this line

builder.setMetadata(metadataMap == null ? Collections.emptyMap() : Collections.synchronizedMap(metadataMap)); should be using METADATA_MAP_SUPPLIER on the else branch? Because this is what happens when there's anything to deserialize already.

why do we need a thread safe implementation here, under any condition the eureka client will try to build or modify the metadata concurrently?

Does not seem like we do, also this uses the same type on the server and the client which makes it a bit unfortunate, as the server may actually modify where as the client should not. At any rate, the existing usage does not require a thread safe map there and already uses regular collections in some places, so I think we're good to change that line to use a regular map.