From 99a4cecec64552a56ec50255ea5d670d91ab4e00 Mon Sep 17 00:00:00 2001 From: Nick Hill Date: Wed, 9 Oct 2019 20:57:54 +0800 Subject: [PATCH] Fix NioEventLoopTest#testChannelsRegistered flakiness (#9650) Motivation NioEventLoopTest#testChannelsRegistered() fails intermittently due to use of SingleThreadEventLoop#channelsRegistered() which is not threadsafe and unreliable when called from outside the event loop. Modifications Add static registeredChannels method to NioEventLoopTest and AbstractSingleThreadEventLoopTest to call from the tests via event loop instead of directly. Result Hopefully fewer test failures --- .../AbstractSingleThreadEventLoopTest.java | 21 ++++++++++++++----- .../netty/channel/nio/NioEventLoopTest.java | 20 ++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/testsuite/src/main/java/io/netty/testsuite/transport/AbstractSingleThreadEventLoopTest.java b/testsuite/src/main/java/io/netty/testsuite/transport/AbstractSingleThreadEventLoopTest.java index fd58a5255b..80442c92e0 100644 --- a/testsuite/src/main/java/io/netty/testsuite/transport/AbstractSingleThreadEventLoopTest.java +++ b/testsuite/src/main/java/io/netty/testsuite/transport/AbstractSingleThreadEventLoopTest.java @@ -20,6 +20,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; @@ -42,7 +43,7 @@ import io.netty.util.concurrent.Future; public abstract class AbstractSingleThreadEventLoopTest { @Test - public void testChannelsRegistered() { + public void testChannelsRegistered() throws Exception { EventLoopGroup group = newEventLoopGroup(); final SingleThreadEventLoop loop = (SingleThreadEventLoop) group.next(); @@ -50,28 +51,38 @@ public abstract class AbstractSingleThreadEventLoopTest { final Channel ch1 = newChannel(); final Channel ch2 = newChannel(); - int rc = loop.registeredChannels(); + int rc = registeredChannels(loop); boolean channelCountSupported = rc != -1; if (channelCountSupported) { - assertEquals(0, loop.registeredChannels()); + assertEquals(0, registeredChannels(loop)); } assertTrue(loop.register(ch1).syncUninterruptibly().isSuccess()); assertTrue(loop.register(ch2).syncUninterruptibly().isSuccess()); if (channelCountSupported) { - assertEquals(2, loop.registeredChannels()); + assertEquals(2, registeredChannels(loop)); } assertTrue(ch1.deregister().syncUninterruptibly().isSuccess()); if (channelCountSupported) { - assertEquals(1, loop.registeredChannels()); + assertEquals(1, registeredChannels(loop)); } } finally { group.shutdownGracefully(); } } + // Only reliable if run from event loop + private static int registeredChannels(final SingleThreadEventLoop loop) throws Exception { + return loop.submit(new Callable() { + @Override + public Integer call() { + return loop.registeredChannels(); + } + }).get(1, TimeUnit.SECONDS); + } + @Test @SuppressWarnings("deprecation") public void shutdownBeforeStart() throws Exception { diff --git a/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java b/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java index c18759dfb7..acc8babd81 100644 --- a/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java +++ b/transport/src/test/java/io/netty/channel/nio/NioEventLoopTest.java @@ -23,6 +23,7 @@ import io.netty.channel.EventLoopGroup; import io.netty.channel.EventLoopTaskQueueFactory; import io.netty.channel.SelectStrategy; import io.netty.channel.SelectStrategyFactory; +import io.netty.channel.SingleThreadEventLoop; import io.netty.channel.socket.ServerSocketChannel; import io.netty.channel.socket.nio.NioServerSocketChannel; import io.netty.util.IntSupplier; @@ -42,6 +43,7 @@ import java.nio.channels.Selector; import java.nio.channels.SocketChannel; import java.nio.channels.spi.SelectorProvider; import java.util.Queue; +import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.RejectedExecutionException; @@ -269,7 +271,7 @@ public class NioEventLoopTest extends AbstractEventLoopTest { @Ignore @Test - public void testChannelsRegistered() { + public void testChannelsRegistered() throws Exception { NioEventLoopGroup group = new NioEventLoopGroup(1); final NioEventLoop loop = (NioEventLoop) group.next(); @@ -277,19 +279,29 @@ public class NioEventLoopTest extends AbstractEventLoopTest { final Channel ch1 = new NioServerSocketChannel(); final Channel ch2 = new NioServerSocketChannel(); - assertEquals(0, loop.registeredChannels()); + assertEquals(0, registeredChannels(loop)); assertTrue(loop.register(ch1).syncUninterruptibly().isSuccess()); assertTrue(loop.register(ch2).syncUninterruptibly().isSuccess()); - assertEquals(2, loop.registeredChannels()); + assertEquals(2, registeredChannels(loop)); assertTrue(ch1.deregister().syncUninterruptibly().isSuccess()); - assertEquals(1, loop.registeredChannels()); + assertEquals(1, registeredChannels(loop)); } finally { group.shutdownGracefully(); } } + // Only reliable if run from event loop + private static int registeredChannels(final SingleThreadEventLoop loop) throws Exception { + return loop.submit(new Callable() { + @Override + public Integer call() { + return loop.registeredChannels(); + } + }).get(1, TimeUnit.SECONDS); + } + @Test public void testCustomQueue() { final AtomicBoolean called = new AtomicBoolean();