andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fixed issue: NETTY-360 Changing Channel.interestOps sometimes does not work under heavy write traffic in NIO transport

Browse Source 
* Made sure all setRawInterestOps() calls are protected by interestOpsLock
Fixed a race condition in the HexDumpProxy example
This commit is contained in:
Trustin Lee 2010-10-24 18:24:20 +00:00
parent d9dba0d754
commit 1f2285f57e
3 changed files with 47 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
src/main/java/org/jboss/netty/channel/socket/nio/NioDatagramWorker.java
View File

@ -604,23 +604,17 @@ class NioDatagramWorker implements Runnable {
close(key); close(key);
return; return;
} }
int interestOps;
boolean changed = false;
// interestOps can change at any time and at any thread. // interestOps can change at any time and at any thread.
// Acquire a lock to avoid possible race condition. // Acquire a lock to avoid possible race condition.
synchronized (channel.interestOpsLock) { synchronized (channel.interestOpsLock) {
interestOps = channel.getRawInterestOps(); int interestOps = channel.getRawInterestOps();
if ((interestOps & SelectionKey.OP_WRITE) == 0) { if ((interestOps & SelectionKey.OP_WRITE) == 0) {
interestOps |= SelectionKey.OP_WRITE; interestOps |= SelectionKey.OP_WRITE;
key.interestOps(interestOps); key.interestOps(interestOps);
changed = true; channel.setRawInterestOpsNow(interestOps);
} }
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
private void clearOpWrite(NioDatagramChannel channel) { private void clearOpWrite(NioDatagramChannel channel) {
@ -633,23 +627,17 @@ class NioDatagramWorker implements Runnable {
close(key); close(key);
return; return;
} }
int interestOps;
boolean changed = false;
// interestOps can change at any time and at any thread. // interestOps can change at any time and at any thread.
// Acquire a lock to avoid possible race condition. // Acquire a lock to avoid possible race condition.
synchronized (channel.interestOpsLock) { synchronized (channel.interestOpsLock) {
interestOps = channel.getRawInterestOps(); int interestOps = channel.getRawInterestOps();
if ((interestOps & SelectionKey.OP_WRITE) != 0) { if ((interestOps & SelectionKey.OP_WRITE) != 0) {
interestOps &= ~SelectionKey.OP_WRITE; interestOps &= ~SelectionKey.OP_WRITE;
key.interestOps(interestOps); key.interestOps(interestOps);
changed = true; channel.setRawInterestOpsNow(interestOps);
} }
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
static void disconnect(NioDatagramChannel channel, ChannelFuture future) { static void disconnect(NioDatagramChannel channel, ChannelFuture future) {
@ -811,11 +799,13 @@ class NioDatagramWorker implements Runnable {
default: default:
throw new Error(); throw new Error();
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
future.setSuccess(); future.setSuccess();
if (changed) { if (changed) {
channel.setRawInterestOpsNow(interestOps);
fireChannelInterestChanged(channel); fireChannelInterestChanged(channel);
} }
} catch (final CancelledKeyException e) { } catch (final CancelledKeyException e) {

25
src/main/java/org/jboss/netty/channel/socket/nio/NioWorker.java
View File

@ -536,23 +536,17 @@ class NioWorker implements Runnable {
close(key); close(key);
return; return;
} }
int interestOps;
boolean changed = false;
// interestOps can change at any time and at any thread. // interestOps can change at any time and at any thread.
// Acquire a lock to avoid possible race condition. // Acquire a lock to avoid possible race condition.
synchronized (channel.interestOpsLock) { synchronized (channel.interestOpsLock) {
interestOps = channel.getRawInterestOps(); int interestOps = channel.getRawInterestOps();
if ((interestOps & SelectionKey.OP_WRITE) == 0) { if ((interestOps & SelectionKey.OP_WRITE) == 0) {
interestOps |= SelectionKey.OP_WRITE; interestOps |= SelectionKey.OP_WRITE;
key.interestOps(interestOps); key.interestOps(interestOps);
changed = true; channel.setRawInterestOpsNow(interestOps);
} }
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
private void clearOpWrite(NioSocketChannel channel) { private void clearOpWrite(NioSocketChannel channel) {
@ -565,23 +559,17 @@ class NioWorker implements Runnable {
close(key); close(key);
return; return;
} }
int interestOps;
boolean changed = false;
// interestOps can change at any time and at any thread. // interestOps can change at any time and at any thread.
// Acquire a lock to avoid possible race condition. // Acquire a lock to avoid possible race condition.
synchronized (channel.interestOpsLock) { synchronized (channel.interestOpsLock) {
interestOps = channel.getRawInterestOps(); int interestOps = channel.getRawInterestOps();
if ((interestOps & SelectionKey.OP_WRITE) != 0) { if ((interestOps & SelectionKey.OP_WRITE) != 0) {
interestOps &= ~SelectionKey.OP_WRITE; interestOps &= ~SelectionKey.OP_WRITE;
key.interestOps(interestOps); key.interestOps(interestOps);
changed = true; channel.setRawInterestOpsNow(interestOps);
} }
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
void close(NioSocketChannel channel, ChannelFuture future) { void close(NioSocketChannel channel, ChannelFuture future) {
@ -719,11 +707,14 @@ class NioWorker implements Runnable {
default: default:
throw new Error(); throw new Error();
} }
if (changed) {
channel.setRawInterestOpsNow(interestOps);
}
} }
future.setSuccess(); future.setSuccess();
if (changed) { if (changed) {
channel.setRawInterestOpsNow(interestOps);
fireChannelInterestChanged(channel); fireChannelInterestChanged(channel);
} }
} catch (CancelledKeyException e) { } catch (CancelledKeyException e) {

51
src/main/java/org/jboss/netty/example/proxy/HexDumpProxyInboundHandler.java
View File

@ -41,6 +41,11 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
private final String remoteHost; private final String remoteHost;
private final int remotePort; private final int remotePort;
// This lock guards against the race condition that overrides the
// OP_READ flag incorrectly.
// See the related discussion: http://markmail.org/message/x7jc6mqx6ripynqf
final Object trafficLock = new Object();
private volatile Channel outboundChannel; private volatile Channel outboundChannel;
public HexDumpProxyInboundHandler( public HexDumpProxyInboundHandler(
@ -78,15 +83,17 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
} }
@Override @Override
public void messageReceived(ChannelHandlerContext ctx, MessageEvent e) public void messageReceived(ChannelHandlerContext ctx, final MessageEvent e)
throws Exception { throws Exception {
ChannelBuffer msg = (ChannelBuffer) e.getMessage(); ChannelBuffer msg = (ChannelBuffer) e.getMessage();
System.out.println(">>> " + ChannelBuffers.hexDump(msg)); //System.out.println(">>> " + ChannelBuffers.hexDump(msg));
outboundChannel.write(msg); synchronized (trafficLock) {
// If outboundChannel is saturated, do not read until notified in outboundChannel.write(msg);
// OutboundHandler.channelInterestChanged(). // If outboundChannel is saturated, do not read until notified in
if (!outboundChannel.isWritable()) { // OutboundHandler.channelInterestChanged().
e.getChannel().setReadable(false); if (!outboundChannel.isWritable()) {
e.getChannel().setReadable(false);
}
} }
} }
@ -95,8 +102,10 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
ChannelStateEvent e) throws Exception { ChannelStateEvent e) throws Exception {
// If inboundChannel is not saturated anymore, continue accepting // If inboundChannel is not saturated anymore, continue accepting
// the incoming traffic from the outboundChannel. // the incoming traffic from the outboundChannel.
if (e.getChannel().isWritable()) { synchronized (trafficLock) {
outboundChannel.setReadable(true); if (e.getChannel().isWritable()) {
outboundChannel.setReadable(true);
}
} }
} }
@ -115,7 +124,7 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
closeOnFlush(e.getChannel()); closeOnFlush(e.getChannel());
} }
private static class OutboundHandler extends SimpleChannelUpstreamHandler { private class OutboundHandler extends SimpleChannelUpstreamHandler {
private final Channel inboundChannel; private final Channel inboundChannel;
@ -124,15 +133,17 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
} }
@Override @Override
public void messageReceived(ChannelHandlerContext ctx, MessageEvent e) public void messageReceived(ChannelHandlerContext ctx, final MessageEvent e)
throws Exception { throws Exception {
ChannelBuffer msg = (ChannelBuffer) e.getMessage(); ChannelBuffer msg = (ChannelBuffer) e.getMessage();
System.out.println("<<< " + ChannelBuffers.hexDump(msg)); //System.out.println("<<< " + ChannelBuffers.hexDump(msg));
inboundChannel.write(msg); synchronized (trafficLock) {
// If inboundChannel is saturated, do not read until notified in inboundChannel.write(msg);
// HexDumpProxyInboundHandler.channelInterestChanged(). // If inboundChannel is saturated, do not read until notified in
if (!inboundChannel.isWritable()) { // HexDumpProxyInboundHandler.channelInterestChanged().
e.getChannel().setReadable(false); if (!inboundChannel.isWritable()) {
e.getChannel().setReadable(false);
}
} }
} }
@ -141,8 +152,10 @@ public class HexDumpProxyInboundHandler extends SimpleChannelUpstreamHandler {
ChannelStateEvent e) throws Exception { ChannelStateEvent e) throws Exception {
// If outboundChannel is not saturated anymore, continue accepting // If outboundChannel is not saturated anymore, continue accepting
// the incoming traffic from the inboundChannel. // the incoming traffic from the inboundChannel.
if (e.getChannel().isWritable()) { synchronized (trafficLock) {
inboundChannel.setReadable(true); if (e.getChannel().isWritable()) {
inboundChannel.setReadable(true);
}
} }
} }