ConcurrentModificationException when resetting the server

[Sloy]

Hi! We've been using restmock for a while now, and we're often getting this Exception in many of our builds while running Android UI tests.

We suspect it's happening when we reset the matched requests before a new test, and at the same time there are is request from the previous test still running, because Android shares the process between tests.

More specifically, this happens when we invoke RestMockServer.reset() after a test, and at the same time the method getMatchedRequests() is iterating the matchableCalls list because some background thread made a request after the test already ended.

We try to avoid this by using IdlingResources to make Espresso wait for our background tasks, but having almost 300 tests the probability of leaking some request is quite high.

I can think of an easy solution by replacing the matchableCalls type from LinkedList to CopyOnWriteArrayList.
This might cause the leaked request to not find a matched call and return an http 500 error , but that should be OK since the original test already finished, and I believe that's the currently expected behaviour. It's not a silver bullet solution to flaky tests, but it should help us quite a lot.

What do you think? Is it OK if I open a Pull Request? Or do you think there may be a better approach?

Here's the full stacktrace and some logs of the crash:

D/RESTMock(10912): -> New Request:	GET /api/1/dictionary/professional-level HTTP/1.1
D/RESTMock(10912): ## Removing all responses
I/MockWebServer(10912): MockWebServer[39702] done accepting connections: Socket closed
I/MicrophoneInputStream( 2161): mic_close gzi@1f273041
I/HotwordRecognitionRnr( 2161): Hotword detection finished
I/HotwordRecognitionRnr( 2161): Stopping hotword detection.
I/System.out(10912): path: /api/1/dictionary/professional-level
--------- beginning of crash
E/AndroidRuntime(10912): FATAL EXCEPTION: OkHttp Http2Connection
E/AndroidRuntime(10912): Process: net.infojobs.mobile.android.debug, PID: 10912
E/AndroidRuntime(10912): java.util.ConcurrentModificationException
E/AndroidRuntime(10912): 	at java.util.LinkedList$LinkIterator.next(LinkedList.java:124)
E/AndroidRuntime(10912): 	at io.appflate.restmock.MatchableCallsRequestDispatcher.getMatchedRequests(MatchableCallsRequestDispatcher.java:96)
E/AndroidRuntime(10912): 	at io.appflate.restmock.MatchableCallsRequestDispatcher.dispatch(MatchableCallsRequestDispatcher.java:41)
E/AndroidRuntime(10912): 	at okhttp3.mockwebserver.MockWebServer$Http2SocketHandler.onStream(MockWebServer.java:942)
E/AndroidRuntime(10912): 	at okhttp3.internal.http2.Http2Connection$ReaderRunnable$1.execute(Http2Connection.java:673)
E/AndroidRuntime(10912): 	at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
E/AndroidRuntime(10912): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
E/AndroidRuntime(10912): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
E/AndroidRuntime(10912): 	at java.lang.Thread.run(Thread.java:818)
W/ActivityManager( 1489): Error in app net.infojobs.mobile.android.debug running instrumentation ComponentInfo{net.infojobs.mobile.android.debug.test/com.infojobs.app.testutil.InfojobsTestRunner}:
W/ActivityManager( 1489):   java.util.ConcurrentModificationException
W/ActivityManager( 1489):   java.util.ConcurrentModificationException
D/AndroidRuntime(10900): Shutting down VM

[Sloy]

We've been using a fork of RESTMock with the proposed fix for a few weeks and it has stopped giving such error. We haven't seen any other related issues after that.

Here's the fix: Sloy@1df65ea

Do you think it would be a good idea if I submit a PR?

[andrzejchm]

Thaks for your input!

Wouldn't simple Collections.synchronizedList() work the same way? I think this way would improve code readability as of giving better clue for the purpose of using special type of list (in this case it clearly states we use the thread-safe list that might be accessed from multiple theads).

Anyways, the PR would be very appreciated

[Sloy]

I don't think I've ever used synchronizedList() before, but as far as I know it's slower for reads whereas CopyOnWriteArrayList is slower for writes. I would imagine that a library like RESTMock has many more reads than writes, so CopyOnWriteArrayList seems like a better fit.
But it's totally up to you, let me know if you prefer one over the other.

I'll try to send a PR as soon as I have a bit of spare time. I could also submit some concurrency tests that I wrote to reproduce the issue (and for some reason didn't commit and deleted them 🤦‍♂).

[andrzejchm]

0.4.0 version should contain the abovementioned fix :)