search

perwendel / spark

A simple expressive web framework for java. Spark has a kotlin DSL https://github.com/perwendel/spark-kotlin
Apache License 2.0
9.63k stars 1.56k forks source link

Fixing the ConcurrentModificationException on init #1210

Open kgrech opened 3 years ago

kgrech commented 3 years ago

This PR fixes issue #1118

Context Most of the modern microservices are running behind the orchestrator or load balancer, for example in the Kubernetes cluster. The orchestrator is designed to perform regular checks of the service using the health check API. The response of the health check API is the only way for the orchestrator to know that the is up and running. As soon as the orchestrator starts the service process, it runs an infite loop of the health check requests.

Problem Spark starts listening to the network as soon as the first route is registered. The problem is that the first request can come to the service before all routes are registered: after the first route is registered, but before the last one is registered, at the same time with one of the routes being registered. In this case, Spark will iterate over the routes list (looking for the route to handle the request) at the same time as the list could be modified by initialization. It would cause the following exception and the 500 error code returned to the client as a result:

[qtp168818756-21] ERROR spark.http.matching.GeneralError - 
java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1043)
    at java.base/java.util.ArrayList$Itr.next(ArrayList.java:997)
    at spark.route.Routes.findTargetsForRequestedRoute(Routes.java:215)
    at spark.route.Routes.find(Routes.java:84)
    at spark.http.matching.Routes.execute(Routes.java:34)
    at spark.http.matching.MatcherFilter.doFilter(MatcherFilter.java:134)
    at spark.embeddedserver.jetty.JettyHandler.doHandle(JettyHandler.java:50)
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1586)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
    at org.eclipse.jetty.server.Server.handle(Server.java:516)
    at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:383)
    at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:556)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:375)
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:273)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
    at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:336)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:313)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:171)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:129)
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:375)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:773)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:905)
    at java.base/java.lang.Thread.run(Thread.java:834)

Reproduction steps: Please see the test case included in the PR

Solution: Change the implementation of the routes list inside the Routes class from ArrayList to CopyOnWriteArrayList. Internally, CopyOnWriteArrayList would be copied every time the new element is added, so there is no way it could have ConcurrentModificationException when iterating over it. It does not have any lock inside and hence iteration over it does not have any overhead compared to ArrayList. ConcurrentModificationException has an extra overhead when adding the routes, but since route, registration is one-time action during the service start and most of the microservices should have a relatively low amount of routes, it should not be a concern.

mario-catalan-aurea commented 3 years ago

@kgrech Can I create a PR from this branch to the forked repo I created for my company (https://github.com/trilogy-group/spark-1)?

kgrech commented 3 years ago

@kgrech Can I create a PR from this branch to the forked repo I created for my company (https://github.com/trilogy-group/spark-1)?

Go ahead