Thread-safety issues in DummyInvocationUtils's cache

When upgrading our project from Java 8 to Java 11 we ran into the following failure in our integration testing.

java.util.ConcurrentModificationException at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1134) at org.springframework.hateoas.server.core.DummyInvocationUtils.methodOn(DummyInvocationUtils.java:132) at xxx.xxxxx.xxxxx.controller.assembler.TextSetAssembler.selfLink(TextSetAssembler.java:54) at xxx.xxxxx.xxxxx.controller.assembler.TextSetAssembler.linkToItemResource(TextSetAssembler.java:45) at xxx.xxxxx.xxxxx.controller.assembler.TextSetAssemblerTest.theLinkToSingleResourceShouldReflectIdentityCorrectly(TextSetAssemblerTest.java:47) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:566) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.springframework.test.context.junit4.statements.RunBeforeTestExecutionCallbacks.evaluate(RunBeforeTestExecutionCallbacks.java:74) at org.springframework.test.context.junit4.statements.RunAfterTestExecutionCallbacks.evaluate(RunAfterTestExecutionCallbacks.java:84) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75) at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86) at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:251) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61) at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at org.junit.runner.JUnitCore.run(JUnitCore.java:115) at org.junit.vintage.engine.execution.RunnerExecutor.execute(RunnerExecutor.java:40) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133) at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) at org.junit.vintage.engine.VintageTestEngine.executeAllChildren(VintageTestEngine.java:80) at org.junit.vintage.engine.VintageTestEngine.execute(VintageTestEngine.java:71) at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:220) at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$6(DefaultLauncher.java:188) at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:202) at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:181) at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:128) at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:150) at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:124) at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384) at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345) at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)

This ticket is intentionally named very much like https://github.com/spring-projects/spring-hateoas/issues/1155 since I believe the issue is the same.

It appears that DummyInvocationUtils cache isn't thread safe: private static final ThreadLocal<Map<CacheKey<?>, Object>> CACHE = ThreadLocal.withInitial(HashMap::new);

Version [INFO] +- org.springframework.boot:spring-boot-starter-hateoas:jar:2.2.6.RELEASE:compile [INFO] | - org.springframework.hateoas:spring-hateoas:jar:1.0.4.RELEASE:compile

I’m curious how to reproduce. We have CI testing against JDK 11 and 14.

I think I have found why this happens.

We have an interface SomeApi

public interface SomeApi {

    SomeApi SOME_API_LINKS = methodOn(SomeApi.class);
    ...
}

Then the method called in TextSetAssembler in the test, as seen in the stacktrace, looks like this:

    private Link selfLink(String id) {
        return linkTo(methodOn(SomeApi.class)
                .getTextSet(id
                .withSelfRel();
    }

The linkTo(methodOn(SomeApi.class)) trigger the class loading of SomeApi, which in turn will call methodOn(SomeApi.class) to initialize SOME_API_LINKS.

computeIfAbsent:1133, HashMap (java.util) [2] methodOn:132, DummyInvocationUtils (org.springframework.hateoas.server.core) methodOn:206, WebMvcLinkBuilder (org.springframework.hateoas.server.mvc)

:33, SomeApi (xxx.xxxxx.xxxxx.api) forName0:-1, Class (java.lang) forName:315, Class (java.lang) :-1, $Proxy262 (com.sun.proxy) newInstance0:-1, NativeConstructorAccessorImpl (jdk.internal.reflect) newInstance:62, NativeConstructorAccessorImpl (jdk.internal.reflect) newInstance:45, DelegatingConstructorAccessorImpl (jdk.internal.reflect) newInstance:490, Constructor (java.lang.reflect) newProxyInstance:1022, Proxy (java.lang.reflect) newProxyInstance:1008, Proxy (java.lang.reflect) getProxy:123, JdkDynamicAopProxy (org.springframework.aop.framework) getProxy:110, ProxyFactory (org.springframework.aop.framework) getProxyWithInterceptor:167, DummyInvocationUtils (org.springframework.hateoas.server.core) lambda$methodOn$0:136, DummyInvocationUtils (org.springframework.hateoas.server.core) apply:-1, 1603529730 (org.springframework.hateoas.server.core.DummyInvocationUtils$$Lambda$531) computeIfAbsent:1133, HashMap (java.util) [1] methodOn:132, DummyInvocationUtils (org.springframework.hateoas.server.core) selfLink:54, TextSetAssembler (xxx.xxxxx.xxxxx.controller.assembler) linkToItemResource:45, TextSetAssembler (xxx.xxxxx.xxxxx.controller.assembler) theLinkToSingleResourceShouldReflectIdentityCorrectly:47, TextSetAssemblerTest (xxx.xxxxx.xxxxx.controller.assembler) The class SomeApi is generated. In fact, I think that the purpose of that field is to remove calls to methodOn(SomeApi.class). So by changing TextSetAssembler.selfLink() to `linkTo(SomeApi.SOME_API_LINKS.getTextSet(id))` I get rid of the problem.

I’m leery of depending on one static cache fixing another.

I don't understand your comment, due to my language skill. I agree that our code is a bit smelly in that the static initializer pulls this in. We wanted to avoid linkTo(methodOn(SomeApi.class).getTextSet(id)) in favor something more readable, which might be linkTo(SomeApi.linkToProxy().getTextSet(id)) This could be considered a bug in our code, so I understand if you would leave it as it is.

My apologies. My understanding was that SomeApi is yours, and that somewhere you are performing a linkTo(methodOn(SomeApi.class).getTextSet(id) call, which in a Java 11 environment, triggered a ConcurrentModificationException.

Looking a little closer at our usage of a CACHE, I see a static HashMap wrapped inside a ThreadLocal of our code.

You apparently moved your usage of methodOn(SomeApi.class) into a static declaration, to adjust WHEN that happens, so that it no longer collides with the creation of our CACHE. If so,

I'm worried that timing the creation of statics is a risky venture that might change again in the future, either based on a change in JDK or a change in your own code.

I was trying to ping @odrotbohm, asking if changing CACHE to a ConcurrentHashMap would help. He seems to think that being wrapped inside a ThreadLocal removes the need to do as such.

Your question:

in favor something more readable, which might be linkTo(SomeApi.linkToProxy().getTextSet(id))

I'm not sure I understand the suggestion. Where does linkToProxy() come from given SomeApi is yours? We have written methodOn(Class<?> clazz) in order to wrap a proxy around clazz, and thus trap invocations.

Please note that this ConcurrentModificationException happened in a single thread, so a ThreadLocal won't make any difference. See the stack trace above, where methodOn:132, DummyInvocationUtils appears twice.

So, in the end I think that this case is really an issue on our end. Maybe ConcurrentHashMap would provide a more resilient implementation, but not necessarily. From my part it's OK if you reject this ticket. I haven't digged in to ticket #1155 to understand if this code could be affected under the same conditions as in that ticket.

The rest is a clarification what I did to resolve this on our end.

Original TextAssembler was

    private Link selfLink(String id) {
        return linkTo(methodOn(SomeApi.class)
                .getTextSet(id)
                .withSelfRel();
   }

then I changed TextAssembler implementation to

    private Link selfLink(String id) {
        return linkTo(SomeApi.SOME_API_LINKS
                .getTextSet(id)
                .withSelfRel();
    }

This solution relies on the following code that we always have had there (it's generated by our Swagger Codegen):

public interface SomeApi {
    SomeApi SOME_API_LINKS = methodOn(SomeApi.class);

which is executing methodOn in the class initialization. One of the benefits of this is that this implicitly caches the proxy globally, since we only ever call methodOn(SomeApi.class) once in the class intitialization.

What I was suggested before was that we in our code could change to:

public interface SomeApi {
    // Remove this - don't invoke methodOn in the class initialization
    // SomeApi SOME_API_LINKS = methodOn(SomeApi.class);

   default SomeApi linkProxy() {
       return methodOn(SomeApi.class);
   }

But when I think about it wouldn't make much sense. Probably wiser to inline that could directly then in that case.

One of the benefits of this is that this implicitly caches the proxy globally, since we only ever call methodOn(SomeApi.class) once in the class intitialization.

That's not a benefit, it's a problem. The proxy contains a method interceptor that records the method invocation, i.e. it keeps state. If multiple threads call methods on it they will inevitably overwrite that state. That's why we introduced the thread scoped cache: we know the state is immediately consumed in a linkTo(…) method so it's fine to keep the proxy around for the current thread.

spring-projects / spring-hateoas

Thread-safety issues in DummyInvocationUtils's cache #1283