Race condition causes ConcurrentModificationException when binding parameters shortly after application startup

[davidcostanzo]

Play Version

1.7.1

Describe the bug

After upgrading our applications from Play 1.6.0 to Play 1.7.1, we noticed some rare exceptions binding HTTP request parameters to Java variables.

The exception looks like this

Failed to bind myParameter=[MyValue]
java.util.ConcurrentModificationException
	at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1221)
	at play.classloading.ApplicationClassloader.getAssignableClasses(ApplicationClassloader.java:481)
	at play.data.binding.Binder.internalDirectBind(Binder.java:691)
	at play.data.binding.Binder.internalBind(Binder.java:227)
	at play.data.binding.Binder.bind(Binder.java:153)
	at play.mvc.ActionInvoker.getActionMethodArgs(ActionInvoker.java:641)
	at play.mvc.ActionInvoker.invokeControllerMethod(ActionInvoker.java:458)
	at play.mvc.ActionInvoker.invokeControllerMethod(ActionInvoker.java:444)
	at play.mvc.ActionInvoker.invoke(ActionInvoker.java:167)
	at Invocation.HTTP Request(Play!)

I believe this is a rare race condition in Play that can happen if two requests arrive at the same time after a precompiled application starts.

By code inspection, the exception is thrown from Binder.internalDirectBind()

// application custom types have higher priority. If unable to bind proceed with the next one
for (Class<TypeBinder<?>> c : Play.classloader.getAssignableClasses(TypeBinder.class)) {

ApplicationClassloader.getAssignableClasses is implemented as

public List<Class> getAssignableClasses(Class clazz) {
    ...
    return assignableClassesByName.computeIfAbsent(clazz.getName(), className -> {
        List<ApplicationClass> assignableClasses = Play.classes.getAssignableClasses(clazz);
        List<Class<?>> results = new ArrayList<>(assignableClasses.size());
        for (ApplicationClass assignableClass : assignableClasses) {
            results.add(assignableClass.javaClass);
        }
        return unmodifiableList(results);
    });
}

// assignable classes cache
private final Map<String, List<Class>> assignableClassesByName = new HashMap<>(100);

That is, assignableClassesByName is not thread safe, but it's potentially modified concurrently by assignableClassesByName.computeIfAbsent.

It looks like the assignableClassesByName cache was added 8 years ago, but we only started seeing this recently. I suspect that we started seeing this after we upgraded to Play 1.7.1 because of some refactoring that replaced a "get/if(null)/add" pattern with a single invocation of HashMap.computeIfAbsent. The prior code might have had a similar race condition, but it wouldn't have been able to report it and it might not have had any negative impact.

This exception is logged and treated as a data validation error (client error) by Binder.internalBind()

} catch (Exception e) {
    // TODO This is bad catch. I would like to remove it in next version.
    logBindingUnexpectedFailure(paramNode, e);
    addValidationError(paramNode);
}

And then the controller action is invoked using parameters that aren't the ones provided by the client. Depending on the controller action, this could be severe. For example, if this were a POST in a banking app and the missing parameter was part of the instructions.

To Reproduce
Given the nature of this being startup race condition, I don't know if there a solid repro case exists and I don't know if it's possible to write a Junit test that demonstrates the failure.

I expect the repro steps are something like:

1. Precompile a Play 1.7.1 app
2. Start the application.
3. Issues 1000 simultaneous requests for a controller action that takes a String parameter while providing that String in the request.

Expected behavior
All 1000 requests return a 200 ok response. No error are logged by the application.

Desktop (please complete the following information):

• OS: Linux xapps-dev 5.15.0-102-generic #112-Ubuntu SMP Tue Mar 5 16:50:32 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
• Browser: N/A (a web service request made by another Play app)
• JDK [e.g 17]:

openjdk version "17.0.9" 2023-10-17
OpenJDK Runtime Environment Temurin-17.0.9+9 (build 17.0.9+9)
OpenJDK 64-Bit Server VM Temurin-17.0.9+9 (build 17.0.9+9, mixed mode, sharing)

[davidcostanzo]

This was also reported as Issue 1496, which was fixed.