Github user anmolnar commented on a diff in the pull request: https://github.com/apache/zookeeper/pull/669#discussion_r233279457 --- Diff: zookeeper-server/src/main/java/org/apache/zookeeper/server/NettyServerCnxnFactory.java --- @@ -116,170 +116,104 @@ public void channelConnected(ChannelHandlerContext ctx, NettyServerCnxn cnxn = new NettyServerCnxn(channel, zkServer, NettyServerCnxnFactory.this); - ctx.setAttachment(cnxn); + ctx.channel().attr(CONNECTION_ATTRIBUTE).set(cnxn); if (secure) { - SslHandler sslHandler = ctx.getPipeline().get(SslHandler.class); - ChannelFuture handshakeFuture = sslHandler.handshake(); + SslHandler sslHandler = ctx.pipeline().get(SslHandler.class); + Future<Channel> handshakeFuture = sslHandler.handshakeFuture(); handshakeFuture.addListener(new CertificateVerifier(sslHandler, cnxn)); } else { - allChannels.add(ctx.getChannel()); + allChannels.add(ctx.channel()); addCnxn(cnxn); } } @Override - public void channelDisconnected(ChannelHandlerContext ctx, - ChannelStateEvent e) throws Exception - { + public void channelInactive(ChannelHandlerContext ctx) throws Exception { if (LOG.isTraceEnabled()) { - LOG.trace("Channel disconnected " + e); + LOG.trace("Channel inactive {}", ctx.channel()); } - NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment(); + allChannels.remove(ctx.channel()); + NettyServerCnxn cnxn = ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null); if (cnxn != null) { if (LOG.isTraceEnabled()) { - LOG.trace("Channel disconnect caused close " + e); + LOG.trace("Channel inactive caused close {}", cnxn); } cnxn.close(); } } @Override - public void exceptionCaught(ChannelHandlerContext ctx, ExceptionEvent e) - throws Exception - { - LOG.warn("Exception caught " + e, e.getCause()); - NettyServerCnxn cnxn = (NettyServerCnxn) ctx.getAttachment(); + public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception { + LOG.warn("Exception caught", cause); + NettyServerCnxn cnxn = ctx.channel().attr(CONNECTION_ATTRIBUTE).getAndSet(null); if (cnxn != null) { if (LOG.isDebugEnabled()) { - LOG.debug("Closing " + cnxn); + LOG.debug("Closing {}", cnxn); } cnxn.close(); } } @Override - public void messageReceived(ChannelHandlerContext ctx, MessageEvent e) - throws Exception - { - if (LOG.isTraceEnabled()) { - LOG.trace("message received called " + e.getMessage()); - } + public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception { try { - if (LOG.isDebugEnabled()) { - LOG.debug("New message " + e.toString() - + " from " + ctx.getChannel()); - } - NettyServerCnxn cnxn = (NettyServerCnxn)ctx.getAttachment(); - synchronized(cnxn) { - processMessage(e, cnxn); + if (evt == NettyServerCnxn.AutoReadEvent.ENABLE) { + LOG.debug("Received AutoReadEvent.ENABLE"); + NettyServerCnxn cnxn = ctx.channel().attr(CONNECTION_ATTRIBUTE).get(); + // TODO(ilyam): Not sure if cnxn can be null here. It becomes null if channelInactive() --- End diff -- Do you need to remove `cnxn` from the channel in the mentioned two events? Null check wouldn't do any harm though.
---