Guard against ConcurrentModificationException when the response processes commitActions

[libetl]

Affects: <Spring Framework version>v5.3.8

---

• Version : spring-web v5.3.8
• Server type : reactive
• Java class : org.springframework.http.server.reactive.AbstractServerHttpResponse
• Method : protected Mono<Void> doCommit(@Nullable Supplier<? extends Mono<Void>> writeAction) {

Hello,

Sometimes there is a race condition between beforeCommit statements for the same request.
This error only arises during traffic peaks

Both HttpHeaderWriterWebFilter and DefaultWebSessionManager add commitActions to the response
https://github.com/spring-projects/spring-security-reactive/blob/37749a64f782c2b2f81afb3db1b30cea3e956839/spring-security-reactive/src/main/java/org/springframework/security/web/server/header/HttpHeaderWriterWebFilter.java#L42

spring-framework/spring-web/src/main/java/org/springframework/web/server/session/DefaultWebSessionManager.java

Line 88 in b595dc1

.doOnNext(session -> exchange.getResponse().beforeCommit(() -> save(exchange, session))));

But sometimes (fortunately it is not happening a lot) DefaultWebSessionManager adds its commitAction after the method doCommit has started.

As a result,
doCommit does Flux.concat(Flux.fromIterable(this.commitActions).map(Supplier::get)), which immediately stores an iterator over commitActions.

spring-framework/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java

Line 243 in 9c33d27

allActions = Flux.concat(Flux.fromIterable(this.commitActions).map(Supplier::get))

Then DefaultWebSessionManager adds its commitAction,
which means that this.commitActions has size 2, but the iterator was built for a list of 1.

When the FluxIterable subscribes, it does iterator.next, which invokes ArrayList$Itr.checkForComodification
This is where I get the ConcurrentModificationException .

To illustrate the problem, I am providing you this test class in kotlin.
This test systematically fails, but there is no race condition, it is simply a beforeCommit that takes place during another one, making the code build an iterator on a list of 1 when the list size increases to 2.

package mypackage

import org.junit.jupiter.api.Test
import org.springframework.mock.http.server.reactive.MockServerHttpResponse
import reactor.core.publisher.Mono

class AbstractServerHttpResponseTest {

    @Test
    fun test1() {
        MockServerHttpResponse()
            .apply {
                beforeCommit {
                    beforeCommit {
                        Mono.empty()
                    }
                    Mono.empty()
                }
            }
            .setComplete().block();
    }
}

If it is an app-level error, can we think of a better error handling to avoid such problems ?
Otherwise, if it is a framework-level error, I would be happy to contribute, just let me know.

[libetl]

If it is a framework problem, we could try using a different primitive instead of Flux.fromIterable, so even if there are race conditions, it can accept the entire list of commit actions.

[rstoyanchev]

I realize the above snippet is just a way to cause the error, but I do wonder if the root cause is a race condition where the WebSession is used while the response is being committed, or a pre-commit action that itself uses the WebSession.

For the former, we should arguably reject a concurrent registration, in the call to beforeCommit, because the registration otherwise may or may not succeed, depending on exact timing. Generally, the response is committed fairly late, after the controller returns and the response body is written, or at the end of the filter chain, and things happen in order thanks to the reactive chain declaration, so it's a bit hard to picture a concurrent issue but not ruling it out.

For the latter, i think there is a case to allow it, but it'd be useful to confirm that's what happens. I've looked at Spring Security header writers and don't see anything trying to use the WebSession, but I could ask @rwinch or @jzheaux to comment whether Spring Security registers any preCommit action with the response that might in turn use the WebSession internally?

[sbrannen]

@rwinch and @jzheaux, can you please comment on the above question?

whether Spring Security registers any preCommit action with the response that might in turn use the WebSession internally?

[rwinch]

Spring Security does not register any beforeCommit action that uses the WebSession internally.

[rstoyanchev]

Unfortunately not possible to determine the root cause. I think the best we can do is to reject further commitActions from being added when the response is already committed, or ignore them, e.g. by using CopyOnWriteArrayList.

Generally, if all request handling is composed in one reactive chain, response writing should happen at the end, and there shouldn't be any competition. The fact that there is implies that there is either a component that executes asynchronously without being composed into the request handling chain (e.g. started with an explicit call to subscribe), or that the response is written early by some asynchronous component (e.g. WebFilter) integrated with flatMap into the request handling chain.

[rstoyanchev]

On closer look, the above test with the nested beforeCommit action no longer fails. The nested action is simply ignored. There have a been a number of improvements in FluxIterable that likely account for this. I have replaced ArrayList with CopyOnWriteArrayList in any case to avoid ConcurrentModificationException.