Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing the ConcurrentModificationException on init #1210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kgrech
Copy link

@kgrech kgrech commented Mar 4, 2021

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
Copy link

@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
Copy link
Author

kgrech commented Mar 9, 2021

@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

@lepe lepe mentioned this pull request Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants