Address ConcurrentModificationException on ThreadLocalPool #14511
Conversation
…ndoned connections in ThreadLocalPool.
| @@ -72,7 +72,7 @@ PoolType pool() { | |||
| } | |||
|
|
|||
| private final void scanForAbandonedConnections() { | |||
| for (PoolAndThread pair : allConnections) { | |||
| for (PoolAndThread pair : new ArrayList<>(allConnections)) { | |||
There was a problem hiding this comment.
I think this may still run into ConcurrentModificationException (when copying to the other list).
A proper solution is to replace the ArrayList instance used in allConnections with a CopyOnWriteArrayList
There was a problem hiding this comment.
I agree - just pushed a small additional commit.
There was a problem hiding this comment.
(I used a slightly different approach as I actually did it before seeing your suggestion)
There was a problem hiding this comment.
And it's actually still not 100% right..
There was a problem hiding this comment.
Nevermind, should be correct as all accesses to this method are exclusive because of the synchronization pattern.
Sanne
added
triage/waiting-for-ci
| @@ -72,13 +72,19 @@ PoolType pool() { | |||
| } | |||
|
|
|||
| private final void scanForAbandonedConnections() { | |||
| ArrayList<PoolAndThread> garbage = new ArrayList<>(); | |||
| for (PoolAndThread pair : allConnections) { | |||
There was a problem hiding this comment.
This iteration may still fail if the allConnections list changes
There was a problem hiding this comment.
it's locked, it's not going to change at this time.
|
only a JDK8/JVM test failed, and it was: That's not related, so let's merge this one. |
|
Many thanks @nachogiljaldo ! |
Fixes #14510
I tried to keep the solution super-simple since I saw this code might go away real soon, I added a test that reproduces.
IMHO, there should be a 1.11.1 with this change, this seems like a deal breaker for users in that version (I had to move back to 1.10.5.FINAL for example).