From dev-return-78216-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Wed Feb 6 15:59:55 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 0E70C18067A for ; Wed, 6 Feb 2019 16:59:54 +0100 (CET) Received: (qmail 6701 invoked by uid 500); 6 Feb 2019 15:59:54 -0000 Mailing-List: contact dev-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list dev@zookeeper.apache.org Received: (qmail 6678 invoked by uid 99); 6 Feb 2019 15:59:54 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Feb 2019 15:59:54 +0000 From: GitBox To: dev@zookeeper.apache.org Subject: [GitHub] ivmaykov commented on a change in pull request #753: ZOOKEEPER-3204: Reconfig tests are constantly failing on 3.5 after applying Java 11 fix Message-ID: <154946879356.16447.8485741261594083128.gitbox@gitbox.apache.org> Date: Wed, 06 Feb 2019 15:59:53 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit ivmaykov commented on a change in pull request #753: ZOOKEEPER-3204: Reconfig tests are constantly failing on 3.5 after applying Java 11 fix URL: https://github.com/apache/zookeeper/pull/753#discussion_r254332442 ########## File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxnSocketNetty.java ########## @@ -104,71 +106,102 @@ boolean isConnected() { // Assuming that isConnected() is only used to initiate connection, // not used by some other connection status judgement. - return channel != null; + connectLock.lock(); + try { + return channel != null || connectFuture != null; + } finally { + connectLock.unlock(); + } + } + + private Bootstrap configureBootstrapAllocator(Bootstrap bootstrap) { + ByteBufAllocator testAllocator = TEST_ALLOCATOR.get(); + if (testAllocator != null) { + return bootstrap.option(ChannelOption.ALLOCATOR, testAllocator); + } else { + return bootstrap; + } } @Override void connect(InetSocketAddress addr) throws IOException { firstConnect = new CountDownLatch(1); - ClientBootstrap bootstrap = new ClientBootstrap(channelFactory); - - bootstrap.setPipelineFactory(new ZKClientPipelineFactory(addr.getHostString(), addr.getPort())); - bootstrap.setOption("soLinger", -1); - bootstrap.setOption("tcpNoDelay", true); - - connectFuture = bootstrap.connect(addr); - connectFuture.addListener(new ChannelFutureListener() { - @Override - public void operationComplete(ChannelFuture channelFuture) throws Exception { - // this lock guarantees that channel won't be assgined after cleanup(). - connectLock.lock(); - try { - if (!channelFuture.isSuccess() || connectFuture == null) { - LOG.info("future isn't success, cause: {}", channelFuture.getCause()); - return; - } - // setup channel, variables, connection, etc. - channel = channelFuture.getChannel(); - - disconnected.set(false); - initialized = false; - lenBuffer.clear(); - incomingBuffer = lenBuffer; - - sendThread.primeConnection(); - updateNow(); - updateLastSendAndHeard(); - - if (sendThread.tunnelAuthInProgress()) { - waitSasl.drainPermits(); - needSasl.set(true); - sendPrimePacket(); - } else { - needSasl.set(false); - } + Bootstrap bootstrap = new Bootstrap() + .group(eventLoopGroup) + .channel(NettyUtils.nioOrEpollSocketChannel()) + .option(ChannelOption.SO_LINGER, -1) + .option(ChannelOption.TCP_NODELAY, true) + .handler(new ZKClientPipelineFactory(addr.getHostString(), addr.getPort())); + bootstrap = configureBootstrapAllocator(bootstrap); + bootstrap.validate(); - // we need to wake up on first connect to avoid timeout. - wakeupCnxn(); - firstConnect.countDown(); - LOG.info("channel is connected: {}", channelFuture.getChannel()); - } finally { - connectLock.unlock(); + connectLock.lock(); + try { + connectFuture = bootstrap.connect(addr); + connectFuture.addListener(new ChannelFutureListener() { + @Override + public void operationComplete(ChannelFuture channelFuture) throws Exception { + // this lock guarantees that channel won't be assigned after cleanup(). + connectLock.lock(); + try { + if (!channelFuture.isSuccess()) { + LOG.info("future isn't success, cause:", channelFuture.cause()); + return; + } else if (connectFuture == null) { + LOG.info("connect attempt cancelled"); + // If the connect attempt was cancelled but succeeded + // anyway, make sure to close the channel, otherwise + // we may leak a file descriptor. + channelFuture.channel().close(); + return; + } + // setup channel, variables, connection, etc. + channel = channelFuture.channel(); + + disconnected.set(false); + initialized = false; + lenBuffer.clear(); + incomingBuffer = lenBuffer; Review comment: I think I would rather not, for readability. It may not be obvious to someone who is not a netty expert that `ByteBuf.clear()` returns the `ByteBuf`. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org With regards, Apache Git Services