Fix ConnectionManagerTests (#44769) (#44789)

* In both fake connection validators we were potentially executing the listener twice. This lead to the situation that the locking via `connectionLock` that ensures that each listener is only executed once ever
would fail and the lister would run twice (in which case the listeners for that node are already `null` and we get an NPE)
* The fact that two different tests fail is due to the fact that we weren't safely shutting down the threadpool which meant the the task that trips the assertion (on the generic pool) would leak into the next test and fail it
* Closes #44758
This commit is contained in:
Armin Braun 2019-07-24 13:12:57 +02:00 committed by GitHub
parent c329b454d9
commit 4a3218551c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
1 changed files with 4 additions and 3 deletions

View File

@ -37,6 +37,7 @@ import java.util.List;
import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.BrokenBarrierException;
import java.util.concurrent.CountDownLatch; import java.util.concurrent.CountDownLatch;
import java.util.concurrent.CyclicBarrier; import java.util.concurrent.CyclicBarrier;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.AtomicReference;
@ -68,7 +69,7 @@ public class ConnectionManagerTests extends ESTestCase {
@After @After
public void stopThreadPool() { public void stopThreadPool() {
threadPool.shutdown(); ThreadPool.terminate(threadPool, 10L, TimeUnit.SECONDS);
} }
public void testConnectAndDisconnect() { public void testConnectAndDisconnect() {
@ -130,7 +131,7 @@ public class ConnectionManagerTests extends ESTestCase {
ActionListener<Transport.Connection> listener = (ActionListener<Transport.Connection>) invocationOnMock.getArguments()[2]; ActionListener<Transport.Connection> listener = (ActionListener<Transport.Connection>) invocationOnMock.getArguments()[2];
if (rarely()) { if (rarely()) {
listener.onResponse(connection); listener.onResponse(connection);
} if (frequently()) { } else if (frequently()) {
threadPool.generic().execute(() -> listener.onResponse(connection)); threadPool.generic().execute(() -> listener.onResponse(connection));
} else { } else {
threadPool.generic().execute(() -> listener.onFailure(new IllegalStateException("dummy exception"))); threadPool.generic().execute(() -> listener.onFailure(new IllegalStateException("dummy exception")));
@ -143,7 +144,7 @@ public class ConnectionManagerTests extends ESTestCase {
ConnectionManager.ConnectionValidator validator = (c, p, l) -> { ConnectionManager.ConnectionValidator validator = (c, p, l) -> {
if (rarely()) { if (rarely()) {
l.onResponse(null); l.onResponse(null);
} if (frequently()) { } else if (frequently()) {
threadPool.generic().execute(() -> l.onResponse(null)); threadPool.generic().execute(() -> l.onResponse(null));
} else { } else {
threadPool.generic().execute(() -> l.onFailure(new IllegalStateException("dummy exception"))); threadPool.generic().execute(() -> l.onFailure(new IllegalStateException("dummy exception")));