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

Fixes #2389 : Parallel use of mocks with deep stubbing may lead to ConcurrentModificationException #2444

Merged
merged 1 commit into from Oct 13, 2021

Conversation

@LeMikaelF
Copy link
Contributor

@LeMikaelF LeMikaelF commented Oct 12, 2021

Fixes #2389 "Parallel use of mocks with deep stubbing may lead to ConcurrentModificationException"

The issue was that ReturnsDeepStubs (line 82) retrieved the existings stubbings via InvocationContainerImpl::getStubbingsDescending, and iterating on it to find existing stubbings resulted in a ConcurrentModificationException because other threads were adding their own Answer to the stubbings list (InvocationContainerImpl::addAnswer called at ReturnsDeepStubs line 150).

All of the methods in InvocationContainerImpl that mutate the LinkedList<StubbedInvocationMatcher> stubbed field are synchronized on stubbed, but the getters aren't (getStubbingsAscending and getStubbingsDescending). I added a InvocationMatcherImpl::findStubbedAnswer that does basically what ReturnsDeepStubs used to do, but synchronizing on stubbed.

Somebody had been there before and had noticed there was something weird, because there was a TODO in the code:

// TODO why don't we do container.findAnswer here?

However, I tried that in my original solution (using findAnswerFor()), but it didn't work because it used the matches() method from the elements of stubbed, which don't have the matchers from verify() (see test and debugger screenshots below).
image
image

I also added a test for regressions.

EDIT: described new solution

@LeMikaelF
Copy link
Contributor Author

@LeMikaelF LeMikaelF commented Oct 12, 2021

I see the CI tests are failing. I ran ./gradlew check before pushing this, and didn't get any error. I'll look into it some more tomorrow.

@LeMikaelF LeMikaelF force-pushed the fix-concurrent-modification branch from 8ef1562 to fc5e0b2 Oct 12, 2021
@LeMikaelF
Copy link
Contributor Author

@LeMikaelF LeMikaelF commented Oct 12, 2021

I pushed a new implementation for this. Everything passed locally, the CI is still failing but it looks like it's not related to the tests or application code:

Publishing build scan...
https://gradle.com/s/r5ulppd5d6ofq
 
/dev/fd/63: line 1: $'{\r': command not found
/dev/fd/63: line 2: type:: command not found
/dev/fd/63: line 3: message:: command not found
/dev/fd/63: line 4: syntax error near unexpected token }' /dev/fd/63: line 4: }'
Error: Process completed with exit code 2.

I will be happy to address any comments on this PR.

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Oct 13, 2021

Yeah that must be a flake in Gradle infrastructure. I have restarted the jobs, hopefully that fixes the issue.

@TimvdLippe TimvdLippe merged commit b77aee1 into mockito:main Oct 13, 2021
7 checks passed
@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Oct 13, 2021

@LeMikaelF Thank you so much for your contribution and fix!

@LeMikaelF LeMikaelF deleted the fix-concurrent-modification branch Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants