From a9948d681ee667dfa2e2b9252bd10fc899c08dae Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Tue, 29 May 2012 17:25:09 -0700 Subject: [PATCH] Throw NoSuchBufferException instead of returning null - Exception in this case makes a user less confusing - To reduce the overhead of filling the stack trace, NoSuchBufferException has a public pre-constructed instance. - This is necessary because codec framework sometimes need to support both type of outbound buffers. - Fixed a bug where SpdyFrameEncoder did not handle ping messages - Reduced memory copy in codec embedder (EmbeddedChannel) --- .../handler/codec/spdy/SpdyFrameEncoder.java | 3 +- .../io/netty/handler/codec/CodecUtil.java | 31 ++++++++-------- .../codec/embedder/EmbeddedChannel.java | 9 ++--- .../io/netty/channel/ChannelBufferHolder.java | 34 ++++++++++++++---- .../netty/channel/DefaultChannelPipeline.java | 8 ++--- .../netty/channel/NoSuchBufferException.java | 36 +++++++++++++++++++ 6 files changed, 87 insertions(+), 34 deletions(-) create mode 100644 transport/src/main/java/io/netty/channel/NoSuchBufferException.java diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameEncoder.java index 1511b88d8c..ebba732500 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameEncoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdyFrameEncoder.java @@ -84,7 +84,8 @@ public class SpdyFrameEncoder extends MessageToStreamEncoder { msg instanceof SpdySettingsFrame || msg instanceof SpdyNoOpFrame || msg instanceof SpdyGoAwayFrame || - msg instanceof SpdyHeadersFrame; + msg instanceof SpdyHeadersFrame || + msg instanceof SpdyPingFrame; } @Override diff --git a/codec/src/main/java/io/netty/handler/codec/CodecUtil.java b/codec/src/main/java/io/netty/handler/codec/CodecUtil.java index 6bc51e8044..ee057cd122 100644 --- a/codec/src/main/java/io/netty/handler/codec/CodecUtil.java +++ b/codec/src/main/java/io/netty/handler/codec/CodecUtil.java @@ -2,8 +2,7 @@ package io.netty.handler.codec; import io.netty.buffer.ChannelBuffer; import io.netty.channel.ChannelHandlerContext; - -import java.util.Queue; +import io.netty.channel.NoSuchBufferException; class CodecUtil { @@ -33,34 +32,32 @@ class CodecUtil { } if (inbound) { - Queue dst = ctx.nextInboundMessageBuffer(); - if (dst != null) { - dst.add(msg); + try { + ctx.nextInboundMessageBuffer().add(msg); return true; - } else if (msg instanceof ChannelBuffer) { - ChannelBuffer altDst = ctx.nextInboundByteBuffer(); - ChannelBuffer src = (ChannelBuffer) msg; - if (altDst != null) { + } catch (NoSuchBufferException e) { + if (msg instanceof ChannelBuffer) { + ChannelBuffer altDst = ctx.nextInboundByteBuffer(); + ChannelBuffer src = (ChannelBuffer) msg; altDst.writeBytes(src, src.readerIndex(), src.readableBytes()); return true; } } } else { - Queue dst = ctx.nextOutboundMessageBuffer(); - if (dst != null) { - dst.add(msg); + try { + ctx.nextOutboundMessageBuffer().add(msg); return true; - } else if (msg instanceof ChannelBuffer) { - ChannelBuffer altDst = ctx.nextOutboundByteBuffer(); - ChannelBuffer src = (ChannelBuffer) msg; - if (altDst != null) { + } catch (NoSuchBufferException e) { + if (msg instanceof ChannelBuffer) { + ChannelBuffer altDst = ctx.nextOutboundByteBuffer(); + ChannelBuffer src = (ChannelBuffer) msg; altDst.writeBytes(src, src.readerIndex(), src.readableBytes()); return true; } } } - throw new IllegalStateException("no suitable destination buffer found"); + throw new NoSuchBufferException(); } private CodecUtil() { diff --git a/codec/src/main/java/io/netty/handler/codec/embedder/EmbeddedChannel.java b/codec/src/main/java/io/netty/handler/codec/embedder/EmbeddedChannel.java index 019f17f234..da158fe051 100644 --- a/codec/src/main/java/io/netty/handler/codec/embedder/EmbeddedChannel.java +++ b/codec/src/main/java/io/netty/handler/codec/embedder/EmbeddedChannel.java @@ -16,6 +16,7 @@ package io.netty.handler.codec.embedder; import io.netty.buffer.ChannelBuffer; +import io.netty.buffer.ChannelBuffers; import io.netty.channel.AbstractChannel; import io.netty.channel.ChannelBufferHolder; import io.netty.channel.ChannelBufferHolders; @@ -39,7 +40,7 @@ class EmbeddedChannel extends AbstractChannel { EmbeddedChannel(Queue productQueue) { super(null, null); this.productQueue = productQueue; - firstOut = ChannelBufferHolders.catchAllBuffer(); + firstOut = ChannelBufferHolders.catchAllBuffer(productQueue, ChannelBuffers.dynamicBuffer()); } @Override @@ -111,11 +112,7 @@ class EmbeddedChannel extends AbstractChannel { productQueue.add(byteBuf.readBytes(byteBufLen)); byteBuf.clear(); } - Queue msgBuf = buf.messageBuffer(); - if (!msgBuf.isEmpty()) { - productQueue.addAll(msgBuf); - msgBuf.clear(); - } + // We do nothing for message buffer because it's actually productQueue. } @Override diff --git a/transport/src/main/java/io/netty/channel/ChannelBufferHolder.java b/transport/src/main/java/io/netty/channel/ChannelBufferHolder.java index 018094c35e..7048464a5f 100644 --- a/transport/src/main/java/io/netty/channel/ChannelBufferHolder.java +++ b/transport/src/main/java/io/netty/channel/ChannelBufferHolder.java @@ -59,9 +59,21 @@ public final class ChannelBufferHolder { case 0: return msgBuf != null; case 1: - return ctx.nextInboundMessageBuffer() != null; + // XXX: Should we introduce hasNext(Inbound|Outbound)(Message|Byte)Buffer() + // just because of this? + try { + ctx.nextInboundMessageBuffer(); + return true; + } catch (NoSuchBufferException e) { + return false; + } case 2: - return ctx.nextOutboundMessageBuffer() != null; + try { + ctx.nextOutboundMessageBuffer(); + return true; + } catch (NoSuchBufferException e) { + return false; + } default: throw new Error(); } @@ -72,9 +84,19 @@ public final class ChannelBufferHolder { case 0: return byteBuf != null; case 1: - return ctx.nextInboundByteBuffer() != null; + try { + ctx.nextInboundByteBuffer(); + return true; + } catch (NoSuchBufferException e) { + return false; + } case 2: - return ctx.nextOutboundByteBuffer() != null; + try { + ctx.nextOutboundByteBuffer(); + return true; + } catch (NoSuchBufferException e) { + return false; + } default: throw new Error(); } @@ -84,7 +106,7 @@ public final class ChannelBufferHolder { switch (bypassDirection) { case 0: if (msgBuf == null) { - throw new IllegalStateException("does not have a message buffer"); + throw new NoSuchBufferException(); } return msgBuf; case 1: @@ -100,7 +122,7 @@ public final class ChannelBufferHolder { switch (bypassDirection) { case 0: if (byteBuf == null) { - throw new IllegalStateException("does not have a byte buffer"); + throw new NoSuchBufferException(); } return byteBuf; case 1: diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java index c32a8bf0c0..e0028f3a68 100644 --- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java +++ b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java @@ -1157,7 +1157,7 @@ public class DefaultChannelPipeline implements ChannelPipeline { for (;;) { ctx = nextInboundContext(ctx.next); if (ctx == null) { - return null; + throw NoSuchBufferException.INSTANCE; } ChannelBufferHolder nextIn = ctx.inbound(); if (nextIn.hasByteBuffer()) { @@ -1172,7 +1172,7 @@ public class DefaultChannelPipeline implements ChannelPipeline { for (;;) { ctx = nextInboundContext(ctx.next); if (ctx == null) { - return null; + throw NoSuchBufferException.INSTANCE; } ChannelBufferHolder nextIn = ctx.inbound(); if (nextIn.hasMessageBuffer()) { @@ -1191,7 +1191,7 @@ public class DefaultChannelPipeline implements ChannelPipeline { if (lastOut.hasByteBuffer()) { return lastOut.byteBuffer(); } else { - return null; + throw NoSuchBufferException.INSTANCE; } } ChannelBufferHolder nextOut = ctx.outbound(); @@ -1211,7 +1211,7 @@ public class DefaultChannelPipeline implements ChannelPipeline { if (lastOut.hasMessageBuffer()) { return lastOut.messageBuffer(); } else { - return null; + throw NoSuchBufferException.INSTANCE; } } ChannelBufferHolder nextOut = ctx.outbound(); diff --git a/transport/src/main/java/io/netty/channel/NoSuchBufferException.java b/transport/src/main/java/io/netty/channel/NoSuchBufferException.java new file mode 100644 index 0000000000..1723861ffc --- /dev/null +++ b/transport/src/main/java/io/netty/channel/NoSuchBufferException.java @@ -0,0 +1,36 @@ +package io.netty.channel; + +/** + * A {@link ChannelPipelineException} which is raised if an inbound or outbound buffer of + * the expected type is not found while transferring data between {@link ChannelHandler}s. + * This exception is usually triggered by an incorrectly configured {@link ChannelPipeline}. + */ +public class NoSuchBufferException extends ChannelPipelineException { + + private static final String DEFAULT_MESSAGE = + "Could not find a suitable destination buffer. Double-check if the pipeline is " + + "configured correctly and its handlers works as expected."; + public static final NoSuchBufferException INSTANCE = new NoSuchBufferException(); + + static { + INSTANCE.setStackTrace(new StackTraceElement[0]); + } + + private static final long serialVersionUID = -131650785896627090L; + + public NoSuchBufferException() { + this(DEFAULT_MESSAGE); + } + + public NoSuchBufferException(String message, Throwable cause) { + super(message, cause); + } + + public NoSuchBufferException(String message) { + super(message); + } + + public NoSuchBufferException(Throwable cause) { + super(cause); + } +}