feedback-to-code/mozzarella huggingface dataset:
Repository: netty/netty
Issue: Suspend-resume in AIO can cause a ReadPendingException 
Labels: defect

When a read completes in AioSocketChannel and the newly read buffer is advertised via fireInboundBufferUpdated(), user code can trigger a ChannelHandlerContext.readable(false), followed by a ChannelHandlerContext.readable(true).

The ChannelHandlerContext.readable(true) causes a new AioSocketChannel.beginRead() call to occur, in this case immediately because we are on the event loop thread, then the fireInboundBufferUpdated() method returns, and the read completion handler calls AioSocketChannel.beginRead() again.

This second call to beginRead() causes the java.nio.channels.ReadPendingException.


Issue comments:
@jfallows merged your pull request.. Thanks again1

Awesome.  Thanks @jfallows!


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java b/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java
index 817cbc0af71..16a44c6c773 100755
--- a/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java
+++ b/transport/src/main/java/io/netty/channel/socket/aio/AioSocketChannel.java
@@ -54,6 +54,7 @@ private static AsynchronousSocketChannel newSocket(AsynchronousChannelGroup grou
     private boolean flushing;
 
     private final AtomicBoolean readSuspended = new AtomicBoolean();
+    private final AtomicBoolean readInProgress = new AtomicBoolean();
 
     private final Runnable readTask = new Runnable() {
         @Override
@@ -191,6 +192,11 @@ private void beginRead() {
             return;
         }
 
+        // prevent ReadPendingException
+        if (!readInProgress.compareAndSet(false, true)) {
+            return;
+        }
+
         ByteBuf byteBuf = pipeline().inboundByteBuffer();
         if (!byteBuf.readable()) {
             byteBuf.discardReadBytes();
@@ -297,6 +303,9 @@ protected void completed0(Integer result, AioSocketChannel channel) {
                     }
                 }
             } finally {
+                // see beginRead
+                channel.readInProgress.set(false);
+
                 if (read) {
                     if (!channel.readSuspended.get()) {
                         pipeline.fireInboundBufferUpdated();


Test patch:
Repository: netty/netty
Issue: HashedWheelTimer.cancel() cannot report failure
Labels: won't fix

As noted by a TODO note in line 505 (branch: 3)

```
    public void cancel() {
        if (!state.compareAndSet(ST_INIT, ST_CANCELLED)) {
            // TODO return false
            return;
        }

        wheel[stopIndex].remove(this);
    }
```

I'll make a pull request to fix this in a moment


Issue comments:
This will break the api. Its fixed in master already but should not
get touched in 3

Sent from my iPhone. Excuse any typos....

Am 09.08.2012 um 01:12 schrieb Cruz Bishop notifications@github.com:

> As noted by a TODO note in line 505 (branch: 3)
> 
> ```
> public void cancel() {
>     if (!state.compareAndSet(ST_INIT, ST_CANCELLED)) {
>         // TODO return false
>         return;
>     }
> 
>     wheel[stopIndex].remove(this);
> }
> ```
> 
> I'll make a pull request to fix this in a moment
> 
> —
> Reply to this email directly or view it on GitHub.


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/util/HashedWheelTimer.java b/src/main/java/org/jboss/netty/util/HashedWheelTimer.java
index 3b8d1d2ae22..fad43274e9a 100644
--- a/src/main/java/org/jboss/netty/util/HashedWheelTimer.java
+++ b/src/main/java/org/jboss/netty/util/HashedWheelTimer.java
@@ -500,13 +500,13 @@ public TimerTask getTask() {
             return task;
         }
 
-        public void cancel() {
+        public boolean cancel() {
             if (!state.compareAndSet(ST_INIT, ST_CANCELLED)) {
-                // TODO return false
-                return;
+                return false;
             }
 
             wheel[stopIndex].remove(this);
+            return true;
         }
 
         public boolean isCancelled() {
diff --git a/src/main/java/org/jboss/netty/util/Timeout.java b/src/main/java/org/jboss/netty/util/Timeout.java
index 162eff87375..bf3ce154ec6 100644
--- a/src/main/java/org/jboss/netty/util/Timeout.java
+++ b/src/main/java/org/jboss/netty/util/Timeout.java
@@ -44,9 +44,11 @@ public interface Timeout {
     boolean isCancelled();
 
     /**
-     * Cancels the {@link TimerTask} associated with this handle.  It the
-     * task has been executed or cancelled already, it will return with no
-     * side effect.
+     * Attempts to cancel the {@link TimerTask} associated with this handle.
+     * If the task has been executed or cancelled already, it will return with
+     * no side effect.
+     *
+     * @return True if the cancellation completed successfully, otherwise false
      */
-    void cancel();
+    boolean cancel();
 }


Test patch:
Repository: netty/netty
Issue: No way to get a UniqueName's ID

There is currently no way to get a raw UniqueName's ID without subclassing.

I'm already working on this, but I thought I should make a bug report anyway.


Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/UniqueName.java b/common/src/main/java/io/netty/util/UniqueName.java
index dbdd47617d9..9eb3e8b4bab 100644
--- a/common/src/main/java/io/netty/util/UniqueName.java
+++ b/common/src/main/java/io/netty/util/UniqueName.java
@@ -55,6 +55,10 @@ public final String name() {
         return name;
     }
 
+    public final int id() {
+        return id;
+    }
+
     @Override
     public final int hashCode() {
         return super.hashCode();


Test patch:
Repository: netty/netty
Issue: netty example: comment error in ObjectEchoClientHandler.java

@bobmcwhirter 
public void messageReceived(ChannelHandlerContext ctx, List<Integer> msg) throws Exception {
        // Echo back the received object to the client.
        ctx.write(msg);
}

does the "to client" in comment should be "to server" ?


Issue comments:
Looks like a copy/paste error :P

I'll go fix that, thanks.


Pull request review comments:

Patch that resolved the issue:
diff --git a/example/src/main/java/io/netty/example/objectecho/ObjectEchoClientHandler.java b/example/src/main/java/io/netty/example/objectecho/ObjectEchoClientHandler.java
index c7b583d99e0..62acef8dddd 100644
--- a/example/src/main/java/io/netty/example/objectecho/ObjectEchoClientHandler.java
+++ b/example/src/main/java/io/netty/example/objectecho/ObjectEchoClientHandler.java
@@ -57,7 +57,7 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {
 
     @Override
     public void messageReceived(ChannelHandlerContext ctx, List<Integer> msg) throws Exception {
-        // Echo back the received object to the client.
+        // Echo back the received object to the server.
         ctx.write(msg);
     }
 


Test patch:
Repository: netty/netty
Issue: netty example: comment error in ObjectEchoClientHandler.java

@bobmcwhirter 
public void messageReceived(ChannelHandlerContext ctx, List<Integer> msg) throws Exception {
        // Echo back the received object to the client.
        ctx.write(msg);
}

does the "to client" in comment should be "to server" ?


Issue comments:
Looks like a copy/paste error :P

I'll go fix that, thanks.


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/example/objectecho/ObjectEchoClientHandler.java b/src/main/java/org/jboss/netty/example/objectecho/ObjectEchoClientHandler.java
index c70aea0590d..61b0ef6d26f 100644
--- a/src/main/java/org/jboss/netty/example/objectecho/ObjectEchoClientHandler.java
+++ b/src/main/java/org/jboss/netty/example/objectecho/ObjectEchoClientHandler.java
@@ -80,7 +80,7 @@ public void channelConnected(
     @Override
     public void messageReceived(
             ChannelHandlerContext ctx, MessageEvent e) {
-        // Echo back the received object to the client.
+        // Echo back the received object to the server.
         transferredMessages.incrementAndGet();
         e.getChannel().write(e.getMessage());
     }


Test patch:
Repository: netty/netty
Issue: AIO server channel accepts channels onto server event loop group
Labels: feature

When a parent `AioServerSocketChannel` accepts a new inbound connection, the child `AioSocketChannel` is created with the server event loop group instead of a separate child event loop group.

Forcing both parent and child channels onto the same event loop group is still functionally correct, but can lead to connection timeout due to starvation at high throughput.  The NIO transport keeps the parent and child event loop groups separate and does not suffer from starvation at high throughput.


Issue comments:
You are right.. Thanks for the report and fix!

However, the accepted AioSocketChannel's AsynchronousChannelGroup is identical to that of AioServerSocketChannel.  It means any async I/O events will be firstly handled by the parent AsyncChannelGroup and then forwarded to the child AsyncChannelGroup.  Do you see any degradation in performance?

Nevermind.  AioEventLoop.execute() will try to avoid that problem using reflection. :-)


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/socket/aio/AioServerSocketChannel.java b/transport/src/main/java/io/netty/channel/socket/aio/AioServerSocketChannel.java
index d7eca8dc7a4..4a83008d7ba 100755
--- a/transport/src/main/java/io/netty/channel/socket/aio/AioServerSocketChannel.java
+++ b/transport/src/main/java/io/netty/channel/socket/aio/AioServerSocketChannel.java
@@ -40,6 +40,7 @@ public class AioServerSocketChannel extends AbstractAioChannel implements Server
     private static final InternalLogger logger =
             InternalLoggerFactory.getInstance(AioServerSocketChannel.class);
 
+    private final AioEventLoopGroup childGroup;
     private final AioServerSocketChannelConfig config;
     private boolean closed;
     private AtomicBoolean readSuspended = new AtomicBoolean();
@@ -60,9 +61,14 @@ private static AsynchronousServerSocketChannel newSocket(AsynchronousChannelGrou
         }
     }
 
-    public AioServerSocketChannel(AioEventLoopGroup eventLoop) {
-        super(null, null, eventLoop, newSocket(eventLoop.group));
-        config = new AioServerSocketChannelConfig(javaChannel());
+    public AioServerSocketChannel(AioEventLoopGroup group) {
+        this(group, group);
+    }
+
+    public AioServerSocketChannel(AioEventLoopGroup parentGroup, AioEventLoopGroup childGroup) {
+        super(null, null, parentGroup, newSocket(parentGroup.group));
+        this.childGroup = childGroup;
+        this.config = new AioServerSocketChannelConfig(javaChannel());
     }
 
     @Override
@@ -147,7 +153,7 @@ protected void completed0(AsynchronousSocketChannel ch, AioServerSocketChannel c
 
             // create the socket add it to the buffer and fire the event
             channel.pipeline().inboundMessageBuffer().add(
-                    new AioSocketChannel(channel, null, channel.group, ch));
+                    new AioSocketChannel(channel, null, channel.childGroup, ch));
             if (!channel.readSuspended.get()) {
                 channel.pipeline().fireInboundBufferUpdated();
             }


Test patch:
Repository: netty/netty
Issue: Encode URI using UTF-8 charset
Labels: defect

Starting with IE7, all URLs are encoded using the UTF-8 character set (http://support.microsoft.com/kb/934279).

Netty assumes that the request uri string is encoded as ASCII -- see HttpRequestEncoder: buf.writeBytes(request.getUri().getBytes("ASCII"))

Requests that are generated by HttpMessageDecoder use StringBuilder to generate the URI and so will not preserve UTF-8 characters when encoded using HttpRequestEncoder.

Since UTF-8 is a strict superset of ASCII, we should be able to encode the URI using UTF-8 without breaking any requests whose URI only contains ASCII characters.


Issue comments:
@jpinner makes sense.. I want to cut 3.5.4.Final tomorrow. Would be nice if you could "fix it" before that..


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/HttpRequestEncoder.java b/src/main/java/org/jboss/netty/handler/codec/http/HttpRequestEncoder.java
index 43d4167af6e..6da12766335 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/HttpRequestEncoder.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/HttpRequestEncoder.java
@@ -37,7 +37,7 @@ protected void encodeInitialLine(ChannelBuffer buf, HttpMessage message) throws
         HttpRequest request = (HttpRequest) message;
         buf.writeBytes(request.getMethod().toString().getBytes("ASCII"));
         buf.writeByte(SP);
-        buf.writeBytes(request.getUri().getBytes("ASCII"));
+        buf.writeBytes(request.getUri().getBytes("UTF-8"));
         buf.writeByte(SP);
         buf.writeBytes(request.getProtocolVersion().toString().getBytes("ASCII"));
         buf.writeByte(CR);


Test patch:
Repository: netty/netty
Issue: pipelining + suspended ChunkedInput triggers StackOverflowError in ChunkedWriteHandler.flush()

We encountered an issue with ChunkedWriteHandler (netty version 3.5.7).

We get StackOverflowError in following scenario:
- ChunkWriteHandler.queue is not empty
- ChunkedWriteHandler.currentEvent is not null and it stores ChunkedInput as message
- chunkedInput stored in currentEvent is in suspended mode (nextChunk returns null but isEndOfInput returns false).

In such case break from while loop in line 248 is executed.
But then immediately after that flush() is called again recursively in line 301, because condition in line 300 evaluates to true.

If chunkedInput is in suspended mode long enough we get StackOverFlowException.

As a fix I moved suspend variable above while loop and modified condition in line 300 to:

```
if (acquired && (!channel.isConnected() || channel.isWritable() && (!queue.isEmpty() && !suspend))) { 
```

I am not exactly sure if it does not break something but it seems to fix an issue.


Issue comments:
Thanks for the detailed analysis and also for the pull-request.. We love contributions :)

I merged your patch in and it will be part of 3.5.8.Final


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/stream/ChunkedWriteHandler.java b/src/main/java/org/jboss/netty/handler/stream/ChunkedWriteHandler.java
index 0cb6d4ef55c..e06c45b4608 100644
--- a/src/main/java/org/jboss/netty/handler/stream/ChunkedWriteHandler.java
+++ b/src/main/java/org/jboss/netty/handler/stream/ChunkedWriteHandler.java
@@ -186,6 +186,7 @@ private void discard(ChannelHandlerContext ctx, boolean fireNow) {
     private void flush(ChannelHandlerContext ctx, boolean fireNow) throws Exception {
         boolean acquired = false;
         final Channel channel = ctx.getChannel();
+        boolean suspend = false;
 
         // use CAS to see if the have flush already running, if so we don't need to take futher actions
         if (acquired = flush.compareAndSet(false, true)) {
@@ -216,7 +217,6 @@ private void flush(ChannelHandlerContext ctx, boolean fireNow) throws Exception
                             final ChunkedInput chunks = (ChunkedInput) m;
                             Object chunk;
                             boolean endOfInput;
-                            boolean suspend;
                             try {
                                 chunk = chunks.nextChunk();
                                 endOfInput = chunks.isEndOfInput();
@@ -297,7 +297,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
 
         }
 
-        if (acquired && (!channel.isConnected() || channel.isWritable() && !queue.isEmpty())) {
+        if (acquired && (!channel.isConnected() || channel.isWritable() && !queue.isEmpty() && !suspend)) {
             flush(ctx, fireNow);
         }
     }


Test patch:
Repository: netty/netty
Issue: HTTP method should be case sensitive

According to the spec method should be case sensitive:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.1

but the case is ignored in HttpMethod.java:

name = name.trim().toUpperCase();

If there are no objections, I'd like to drop the toUpperCase() in both 3.x and 4.0 in order to comply with the spec.


Issue comments:
Go ahead!

Sent from my iPhone. Excuse any typos....

Am 02.11.2012 um 02:21 schrieb Jeff Pinner notifications@github.com:

> According to the spec method should be case sensitive:
> 
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.1
> 
> but the case is ignored in HttpMethod.java:
> 
> name = name.trim().toUpperCase();
> 
> If there are no objections, I'd like to drop the toUpperCase() in both 3.x and 4.0 in order to comply with the spec.
> 
> —
> Reply to this email directly or view it on GitHub.

Both pull request merged... Thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/handler/codec/http/HttpMethod.java b/src/main/java/org/jboss/netty/handler/codec/http/HttpMethod.java
index d8cf627d558..7c8c3b96de2 100644
--- a/src/main/java/org/jboss/netty/handler/codec/http/HttpMethod.java
+++ b/src/main/java/org/jboss/netty/handler/codec/http/HttpMethod.java
@@ -109,7 +109,7 @@ public static HttpMethod valueOf(String name) {
             throw new NullPointerException("name");
         }
 
-        name = name.trim().toUpperCase();
+        name = name.trim();
         if (name.length() == 0) {
             throw new IllegalArgumentException("empty name");
         }
@@ -136,7 +136,7 @@ public HttpMethod(String name) {
             throw new NullPointerException("name");
         }
 
-        name = name.trim().toUpperCase();
+        name = name.trim();
         if (name.length() == 0) {
             throw new IllegalArgumentException("empty name");
         }


Test patch:
Repository: netty/netty
Issue: HTTP method should be case sensitive

According to the spec method should be case sensitive:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.1

but the case is ignored in HttpMethod.java:

name = name.trim().toUpperCase();

If there are no objections, I'd like to drop the toUpperCase() in both 3.x and 4.0 in order to comply with the spec.


Issue comments:
Go ahead!

Sent from my iPhone. Excuse any typos....

Am 02.11.2012 um 02:21 schrieb Jeff Pinner notifications@github.com:

> According to the spec method should be case sensitive:
> 
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.1
> 
> but the case is ignored in HttpMethod.java:
> 
> name = name.trim().toUpperCase();
> 
> If there are no objections, I'd like to drop the toUpperCase() in both 3.x and 4.0 in order to comply with the spec.
> 
> —
> Reply to this email directly or view it on GitHub.

Both pull request merged... Thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpMethod.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpMethod.java
index 6a97242a566..166df645c49 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpMethod.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpMethod.java
@@ -109,7 +109,7 @@ public static HttpMethod valueOf(String name) {
             throw new NullPointerException("name");
         }
 
-        name = name.trim().toUpperCase();
+        name = name.trim();
         if (name.length() == 0) {
             throw new IllegalArgumentException("empty name");
         }
@@ -136,7 +136,7 @@ public HttpMethod(String name) {
             throw new NullPointerException("name");
         }
 
-        name = name.trim().toUpperCase();
+        name = name.trim();
         if (name.length() == 0) {
             throw new IllegalArgumentException("empty name");
         }


Test patch:
Repository: netty/netty
Issue: Minor issue in javadoc of ByteBuf

Inside the section of **NIO Buffers** (generated from master branch), I found it states following:

```
To determine if a buffer can be converted into an NIO buffer, use nioBuffer().
```

Should `nioBufferCount()` be used here instead of `nioBuffer()`?


Issue comments:
@openaphid fixed.. thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/BufType.java b/buffer/src/main/java/io/netty/buffer/BufType.java
index 2a82223a7e9..8b9ffd06740 100644
--- a/buffer/src/main/java/io/netty/buffer/BufType.java
+++ b/buffer/src/main/java/io/netty/buffer/BufType.java
@@ -20,7 +20,7 @@
  */
 public enum BufType {
     /**
-     * Operates one bytes.
+     * Operates on bytes.
      */
     BYTE,
 
diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java b/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java
index b70c9e527ef..9987dc1f014 100755
--- a/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java
+++ b/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java
@@ -64,7 +64,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
     // 'bridge' so that the two handlers access each other's buffer only via the bridges.
     // The content written into a bridge is flushed into the actual buffer by flushBridge().
     //
-    // Note we use an AtomicReferenceFieldUpdater for atomic operations on these to safe memory. This will safe us
+    // Note we use an AtomicReferenceFieldUpdater for atomic operations on these to save memory. This will save us
     // 64 bytes per Bridge.
     @SuppressWarnings("UnusedDeclaration")
     private volatile MessageBridge inMsgBridge;


Test patch: