Fix #239: IdleStateHandler starts two timers

1) ReadTimeoutHandler is also affected by this bug - fixed
2) Reverted IdleStateHandler.beforeAdd() and channelConnected() -
without isAttached() check, timeout can be inaccurate if beforeAdd() is
called long before channelConnected().

handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java

@@ -224,7 +224,15 @@ public class IdleStateHandler extends SimpleChannelUpstreamHandler
 
     @Override
     public void beforeAdd(ChannelHandlerContext ctx) throws Exception {
-        initialize(ctx);
+        if (ctx.getPipeline().isAttached()) {
+            // channelOpen event has been fired already, which means
+            // this.channelOpen() will not be invoked.
+            // We have to initialize here instead.
+            initialize(ctx);
+        } else {
+            // channelOpen event has not been fired yet.
+            // this.channelOpen() will be invoked and initialization will occur there.
+        }
     }
 
     @Override
@@ -242,6 +250,16 @@ public class IdleStateHandler extends SimpleChannelUpstreamHandler
         // NOOP
     }
 
+    @Override
+    public void channelOpen(ChannelHandlerContext ctx, ChannelStateEvent e)
+            throws Exception {
+        // This method will be invoked only if this handler was added
+        // before channelOpen event is fired.  If a user adds this handler
+        // after the channelOpen event, initialize() will be called by beforeAdd().
+        initialize(ctx);
+        ctx.sendUpstream(e);
+    }
+
     @Override
     public void channelClosed(ChannelHandlerContext ctx, ChannelStateEvent e)
             throws Exception {

handler/src/main/java/io/netty/handler/timeout/ReadTimeoutHandler.java

@@ -190,21 +190,46 @@ public class ReadTimeoutHandler extends SimpleChannelUpstreamHandler
     }
 
     private void initialize(ChannelHandlerContext ctx) {
-        State state = new State();
-        ctx.setAttachment(state);
+        State state = state(ctx);
+
+        // Avoid the case where destroy() is called before scheduling timeouts.
+        // See: https://github.com/netty/netty/issues/143
+        if (state.destroyed) {
+            return;
+        }
+
         if (timeoutMillis > 0) {
             state.timeout = timer.newTimeout(new ReadTimeoutTask(ctx), timeoutMillis, TimeUnit.MILLISECONDS);
         }
     }
 
     private void destroy(ChannelHandlerContext ctx) {
-        State state = (State) ctx.getAttachment();
+        State state;
+        synchronized (ctx) {
+            state = state(ctx);
+            state.destroyed = true;
+        }
+
         if (state.timeout != null) {
             state.timeout.cancel();
             state.timeout = null;
         }
     }
 
+    private State state(ChannelHandlerContext ctx) {
+        State state;
+        synchronized (ctx) {
+            // FIXME: It could have been better if there is setAttachmentIfAbsent().
+            state = (State) ctx.getAttachment();
+            if (state != null) {
+                return state;
+            }
+            state = new State();
+            ctx.setAttachment(state);
+        }
+        return state;
+    }
+
     protected void readTimedOut(ChannelHandlerContext ctx) throws Exception {
         Channels.fireExceptionCaught(ctx, EXCEPTION);
     }
@@ -252,6 +277,7 @@ public class ReadTimeoutHandler extends SimpleChannelUpstreamHandler
     private static final class State {
         volatile Timeout timeout;
         volatile long lastReadTime = System.currentTimeMillis();
+        volatile boolean destroyed;
 
         State() {
         }