feedback-to-code/mozzarella huggingface dataset:
Repository: netty/netty
Issue: Spring instantiation of NioEventLoopGroup with 0 number of threads is now complicated
Labels: defect

I used to use the following constructor while instantiating `NioEventLoopGroup` boss and worker threads.    

``` java
/**
     * Create a new instance using nThreads number of threads, the given {@link ThreadFactory} and the
     * {@link SelectorProvider} which is returned by {@link SelectorProvider#provider()}.
     */
    public NioEventLoopGroup(int nThreads, ThreadFactory threadFactory) {
        this(nThreads, threadFactory, SelectorProvider.provider());
    }
```

The issue is that when instantiating from spring, earlier I used to give nThreads = 0 and it would take the default number. But now its throwing `IllegalArgumentException`. Reason I instantiate via spring is so that I can pass in my own `ThreadFactory`.  
It would be good if a condition is kept inside this constructor checking for 0 and then passing the default value of threads. Or have another constructor which takes `ThreadFactory` alone and does the same logic.


Issue comments:
@menacher I wonder why you not can just pass the thread count in by your own ? The problem is that there is not a good "default" value as it really depends on how many NioEventLoopGroups you create and what they do.

@normanmaurer Agreed. But normally when you pass in 0 or -ve to such api it goes for a default silently rather than throwing back an exception. The one use I could see is that when using different machines to deploy same code, sending in 0 automatically ends up using the right number of threads, without me having to tweak them on each machine (for production I will need to tweak for sure, am talking about dev and test).

@menacher I think the problem is that there is no good default as it depends on how many NioEventLoopGroup you create..

Although I agree with @normanmaurer that there's no sane default in most cases, but I accepted @menacher's suggestion because it's also sometimes pretty useful.


Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/nio/NioEventLoopGroup.java b/transport/src/main/java/io/netty/channel/nio/NioEventLoopGroup.java
index 04d89b68fac..d11bb12865f 100644
--- a/transport/src/main/java/io/netty/channel/nio/NioEventLoopGroup.java
+++ b/transport/src/main/java/io/netty/channel/nio/NioEventLoopGroup.java
@@ -58,7 +58,7 @@ public NioEventLoopGroup(int nThreads, ThreadFactory threadFactory) {
      */
     public NioEventLoopGroup(
             int nThreads, ThreadFactory threadFactory, final SelectorProvider selectorProvider) {
-        super(nThreads, threadFactory, selectorProvider);
+        super((nThreads == 0 ? DEFAULT_EVENT_LOOP_THREADS : nThreads), threadFactory, selectorProvider);
     }
 
     /**


Test patch:
Repository: netty/netty
Issue: HttpContentEncoder should not remove Content-Length when acting as a passthrough.
Labels: defect


Issue comments:
Issued pull request #1552.

Will you provide a fix?

Am 10.07.2013 um 20:00 schrieb Jeff Pinner notifications@github.com:

> —
> Reply to this email directly or view it on GitHub.

Yep -- see #1552 :)

@jpinner merged your pullreq in.. Thanks


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
index fb8ec835cab..106e37233a1 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpContentEncoder.java
@@ -123,15 +123,13 @@ protected void encode(ChannelHandlerContext ctx, HttpObject msg, List<Object> ou
                 // If unable to encode, pass through.
                 if (result == null) {
                     if (isFull) {
-                        // As an unchunked response
+                        // Set the content length.
                         res.headers().remove(Names.TRANSFER_ENCODING);
                         res.headers().set(Names.CONTENT_LENGTH, ((ByteBufHolder) res).content().readableBytes());
                         out.add(ReferenceCountUtil.retain(res));
                     } else {
-                        // As a chunked response
-                        res.headers().remove(Names.CONTENT_LENGTH);
-                        res.headers().set(Names.TRANSFER_ENCODING, Values.CHUNKED);
                         out.add(res);
+                        // Pass through all following contents.
                         state = State.PASS_THROUGH;
                     }
                     break;


Test patch:
Repository: netty/netty
Issue: intermittent: local channel sometimes sends messages after close (3.6)

A local channel can receive messages after it has been closed.  This is the result of a race condition in DefaultLocalChannel.flushWriteBuffer:

```
e.getFuture().setSuccess();
fireMessageReceived(pairedChannel, e.getMessage());
fireWriteComplete(this, 1);
```

The future listener in this case is HttpConnectionHandler.ClosingChannelFutureListener.

The problem is that the future.setSuccess() races with the fireMessageReceived.  Moving the setSuccess to happen after the message is sent helps but doesn't completely remove the race:

```
fireMessageReceived(pairedChannel, e.getMessage());
fireWriteComplete(this, 1);
e.getFuture().setSuccess();
```


Issue comments:
duplicate of #1721 


Pull request review comments:
You should move the setSuccess() call before the fireWriteComplete as we always notify the future first before fire this event in other transports


Patch that resolved the issue:
diff --git a/src/main/java/org/jboss/netty/channel/local/DefaultLocalChannel.java b/src/main/java/org/jboss/netty/channel/local/DefaultLocalChannel.java
index da02be3ece8..d127cd4d0a1 100644
--- a/src/main/java/org/jboss/netty/channel/local/DefaultLocalChannel.java
+++ b/src/main/java/org/jboss/netty/channel/local/DefaultLocalChannel.java
@@ -171,8 +171,8 @@ void flushWriteBuffer() {
                                 break;
                             }
 
-                            e.getFuture().setSuccess();
                             fireMessageReceived(pairedChannel, e.getMessage());
+                            e.getFuture().setSuccess();
                             fireWriteComplete(this, 1);
                         }
                     } finally {
@@ -198,8 +198,8 @@ void flushWriteBuffer() {
                     break;
                 }
 
-                e.getFuture().setFailure(cause);
                 fireExceptionCaught(this, cause);
+                e.getFuture().setFailure(cause);
             }
         }
     }


Test patch:
Repository: netty/netty
Issue: The Javadoc example of IdleStateHandler is incorrect

The second param in userEventTriggered() should be converted to IdleStateEvent instead of IdleState.

The following two lines should be changed:
         if (evt instanceof IdleState} {
             IdleState e = (IdleState) evt;


Issue comments:
Checked against examples & submitted PR #1995 

That one can be closed, #1995 is pulled :)

@ifesdjeen true.. thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java b/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java
index 19b8b9c8d90..c69aa7b38e5 100644
--- a/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java
+++ b/handler/src/main/java/io/netty/handler/timeout/IdleStateHandler.java
@@ -74,8 +74,8 @@
  * public class MyHandler extends {@link ChannelDuplexHandler} {
  *     {@code @Override}
  *     public void userEventTriggered({@link ChannelHandlerContext} ctx, {@link Object} evt) throws {@link Exception} {
- *         if (evt instanceof {@link IdleState}} {
- *             {@link IdleState} e = ({@link IdleState}) evt;
+ *         if (evt instanceof {@link IdleStateEvent}} {
+ *             {@link IdleStateEvent} e = ({@link IdleStateEvent}) evt;
  *             if (e.state() == {@link IdleState}.READER_IDLE) {
  *                 ctx.close();
  *             } else if (e.state() == {@link IdleState}.WRITER_IDLE) {


Test patch:
Repository: netty/netty
Issue: JDKZlibEncoder hangs (fails to flush) on large messages
Labels: defect

If the message is great than 8k the message is not flushed.


Issue comments:
Fixed via 8b495bb6e14becb5bda5e8b80257d5d186e18181


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/compression/JdkZlibEncoder.java b/codec/src/main/java/io/netty/handler/codec/compression/JdkZlibEncoder.java
index d0fa3f0b0c7..be806f65210 100644
--- a/codec/src/main/java/io/netty/handler/codec/compression/JdkZlibEncoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/compression/JdkZlibEncoder.java
@@ -212,8 +212,11 @@ protected void encode(ChannelHandlerContext ctx, ByteBuf uncompressed, ByteBuf o
 
         deflater.setInput(inAry);
         while (!deflater.needsInput()) {
-            int numBytes = deflater.deflate(encodeBuf, 0, encodeBuf.length, Deflater.SYNC_FLUSH);
-            out.writeBytes(encodeBuf, 0, numBytes);
+            int numBytes = encodeBuf.length;
+            while (numBytes == encodeBuf.length) {
+                numBytes = deflater.deflate(encodeBuf, 0, encodeBuf.length, Deflater.SYNC_FLUSH);
+                out.writeBytes(encodeBuf, 0, numBytes);
+            }
         }
     }
 


Test patch:
Repository: netty/netty
Issue: %s WebSocket version %s server handshake

1.

In https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java

``` java
logger.debug("%s WebSocket version %s server handshake", channel, version());
```

I guess it should be:

``` java
logger.debug("{} WebSocket version {} server handshake", channel, version());
```

2.

In https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java

``` java
logger.debug("WebSocket version 07 server handshake key: {}, response: %s.", key, accept);
```

I guess it should be:

``` java
logger.debug("WebSocket version 07 server handshake key: {}, response: {}.", key, accept);
```


Issue comments:
@ngocdaothanh would you mind submitting a PR ?

Sure! I wanted to confirm if my guess was right.

I've sent pull requests for branch 4.0, 4.1, and master.


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
index f94b4f79f3e..c2ca118d351 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
@@ -156,7 +156,7 @@ public final ChannelFuture handshake(Channel channel, FullHttpRequest req,
                                             HttpHeaders responseHeaders, final ChannelPromise promise) {
 
         if (logger.isDebugEnabled()) {
-            logger.debug("%s WebSocket version %s server handshake", channel, version());
+            logger.debug("{} WebSocket version {} server handshake", channel, version());
         }
         FullHttpResponse response = newHandshakeResponse(req, responseHeaders);
         ChannelPipeline p = channel.pipeline();
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
index 329f823bd4e..52f461eaccc 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
@@ -111,7 +111,7 @@ protected FullHttpResponse newHandshakeResponse(FullHttpRequest req, HttpHeaders
         String accept = WebSocketUtil.base64(sha1);
 
         if (logger.isDebugEnabled()) {
-            logger.debug("WebSocket version 07 server handshake key: {}, response: %s.", key, accept);
+            logger.debug("WebSocket version 07 server handshake key: {}, response: {}.", key, accept);
         }
 
         res.headers().add(Names.UPGRADE, WEBSOCKET.toLowerCase());


Test patch:
Repository: netty/netty
Issue: %s WebSocket version %s server handshake

1.

In https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java

``` java
logger.debug("%s WebSocket version %s server handshake", channel, version());
```

I guess it should be:

``` java
logger.debug("{} WebSocket version {} server handshake", channel, version());
```

2.

In https://github.com/netty/netty/blob/master/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java

``` java
logger.debug("WebSocket version 07 server handshake key: {}, response: %s.", key, accept);
```

I guess it should be:

``` java
logger.debug("WebSocket version 07 server handshake key: {}, response: {}.", key, accept);
```


Issue comments:
@ngocdaothanh would you mind submitting a PR ?

Sure! I wanted to confirm if my guess was right.

I've sent pull requests for branch 4.0, 4.1, and master.


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
index f94b4f79f3e..c2ca118d351 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java
@@ -156,7 +156,7 @@ public final ChannelFuture handshake(Channel channel, FullHttpRequest req,
                                             HttpHeaders responseHeaders, final ChannelPromise promise) {
 
         if (logger.isDebugEnabled()) {
-            logger.debug("%s WebSocket version %s server handshake", channel, version());
+            logger.debug("{} WebSocket version {} server handshake", channel, version());
         }
         FullHttpResponse response = newHandshakeResponse(req, responseHeaders);
         ChannelPipeline p = channel.pipeline();
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
index c2ede4a11f9..ba751f09f3e 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker07.java
@@ -113,7 +113,7 @@ protected FullHttpResponse newHandshakeResponse(FullHttpRequest req, HttpHeaders
         String accept = WebSocketUtil.base64(sha1);
 
         if (logger.isDebugEnabled()) {
-            logger.debug("WebSocket version 07 server handshake key: {}, response: %s.", key, accept);
+            logger.debug("WebSocket version 07 server handshake key: {}, response: {}.", key, accept);
         }
 
         res.headers().add(Names.UPGRADE, WEBSOCKET);


Test patch:
Repository: netty/netty
Issue: reduce memory usage in ProtobufVarint32LengthFieldPrepender.java
Labels: improvement

Hi

https://github.com/netty/netty/blob/4.0/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java

Now we calculate headerLen for start, and some later create CodedOutputStream with code

```
CodedOutputStream headerOut =
                CodedOutputStream.newInstance(new ByteBufOutputStream(out));
```

default constructor allocate 4096 buffer, but headerLen always less than 5.
More correct way it's set size of buffer for new instance with code:

```
CodedOutputStream headerOut =
                CodedOutputStream.newInstance(new ByteBufOutputStream(out), headerLen);
```

this decrease allocation for header prepender from 4096 bytes to [1-5] bytes for every protobuf message


Issue comments:
We love pull-requests ;)

> Am 24.03.2014 um 13:26 schrieb Alexey notifications@github.com:
> 
> Hi
> 
> https://github.com/netty/netty/blob/4.0/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java
> 
> Now we calculate headerLen for start, and some later create CodedOutputStream with code
> 
> CodedOutputStream headerOut =
>                 CodedOutputStream.newInstance(new ByteBufOutputStream(out));
> default constructor allocate 4096 buffer, but headerLen always less than 5.
> More correct way it's set size of buffer for new instance with code:
> 
> CodedOutputStream headerOut =
>                 CodedOutputStream.newInstance(new ByteBufOutputStream(out), headerLen);
> this decrease allocation for header prepender from 4096 bytes to [1-5] bytes for every protobuf message
> 
> —
> Reply to this email directly or view it on GitHub.

how i can remove first "referenced this issue from a commit in"
because he have incorrect committer and i did't pull request for he =\

pull request: [#2340]

Close it an open a new one? Also please update tge commit message to match tge template we use.

> Am 24.03.2014 um 16:44 schrieb Alexey notifications@github.com:
> 
> how i can remove first pull request,
> because he have incorrect committer and i did't pull request for he =\
> 
> —
> Reply to this email directly or view it on GitHub.


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java b/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java
index b9231781d80..9efd00ff269 100644
--- a/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java
+++ b/codec/src/main/java/io/netty/handler/codec/protobuf/ProtobufVarint32LengthFieldPrepender.java
@@ -47,7 +47,7 @@ protected void encode(
         out.ensureWritable(headerLen + bodyLen);
 
         CodedOutputStream headerOut =
-                CodedOutputStream.newInstance(new ByteBufOutputStream(out));
+                CodedOutputStream.newInstance(new ByteBufOutputStream(out), headerLen);
         headerOut.writeRawVarint32(bodyLen);
         headerOut.flush();
 


Test patch:
Repository: netty/netty
Issue: sun imports should be marked optional

As per the title. sun.security.util and sun.security.x509 should be marked as optional in the manifest.


Issue comments:
@philbaxter thanks merged in :)


Pull request review comments:

Patch that resolved the issue:
diff --git a/pom.xml b/pom.xml
index 7b742b346a4..a215f129fb8 100644
--- a/pom.xml
+++ b/pom.xml
@@ -766,7 +766,7 @@
               <instructions>
                 <Export-Package>${project.groupId}.*</Export-Package>
                 <!-- enforce JVM vendor package as optional -->
-                <Import-Package>sun.misc.*;resolution:=optional,sun.nio.ch;resolution:=optional,*</Import-Package>
+                <Import-Package>sun.misc.*;resolution:=optional,sun.nio.ch;resolution:=optional,sun.security.*;resolution:=optional,*</Import-Package>
                 <!-- override "internal" private package convention -->
                 <Private-Package>!*</Private-Package>
               </instructions>


Test patch:
Repository: netty/netty
Issue: HTTP2 Client Example read losing data

The `onDataRead` method in `Http2ClientConnectionHandler.java` HTTP2 example is incorrectly re-allocating the bytebuf when it needs to be re-sized.


Issue comments:
Pull request accepted...issue closed.


Pull request review comments:

Patch that resolved the issue:
diff --git a/example/src/main/java/io/netty/example/http2/client/Http2ClientConnectionHandler.java b/example/src/main/java/io/netty/example/http2/client/Http2ClientConnectionHandler.java
index d9face6903c..e2c740a245b 100644
--- a/example/src/main/java/io/netty/example/http2/client/Http2ClientConnectionHandler.java
+++ b/example/src/main/java/io/netty/example/http2/client/Http2ClientConnectionHandler.java
@@ -128,8 +128,8 @@ public void onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, in
             collectedData.writeBytes(data);
         } else {
             // Expand the buffer
-            ByteBuf newBuffer = ctx().alloc().buffer(data.readableBytes() + available);
-            newBuffer.writeBytes(data);
+            ByteBuf newBuffer = ctx().alloc().buffer(collectedData.readableBytes() + available);
+            newBuffer.writeBytes(collectedData);
             newBuffer.writeBytes(data);
             collectedData.release();
             collectedData = newBuffer;


Test patch:
Repository: netty/netty
Issue: CompositeByteBuf.deallocate memory improvement
Labels: improvement

In profiling my application, I have noticed that `CompositeByteBuf.deallocate` generates `java.util.ArrayList$Itr` roughly 1.79 GB or 1.40% of the GC pressure during performance testing. This is due to it using the 'enhanced' for-loop.  This can be resolved by converting to the old-style for loop as shown below:

From:

``` java
for (Component c: components) {
    c.freeIfNecessary();
}
```

To:

``` java
int size = components.size();
for (int i = 0; i < size; i++) {
    components.get(i).freeIfNecessary();
}
```

Tomorrow, when I'm more awake, I'll submit a pull request if @normanmaurer or @trustin don't get to it before me :smile: 


Issue comments:
> roughly 1.79 GB or 1.40% of the GC pressure during performance testing

On a curious sidenote, what profiler do you use to get to these numbers?

@jakobbuchgraber you can use Java Mission Control for this.

@jakobbuchgraber I used Java Mission Control's Flight Record to get this data.


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index 818c97569f4..d5cf14c27da 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -1599,8 +1599,11 @@ protected void deallocate() {
         }
 
         freed = true;
-        for (Component c: components) {
-            c.freeIfNecessary();
+        int size = components.size();
+        // We're not using foreach to avoid creating an iterator.
+        // see https://github.com/netty/netty/issues/2642
+        for (int i = 0; i < size; i++) {
+            components.get(i).freeIfNecessary();
         }
 
         if (leak != null) {


Test patch:
Repository: netty/netty
Issue: .gitignore missing bin/ directory
Labels: cleanup

The netty project's .gitignore is missing an entry for `bin/`.  This directory is created by eclipse and maintained by eclipse.  Even though the IDE of choice is intellij the .gitignore should have a `bin/` entry under the Eclipse section to exclude this directory.


Issue comments:
PR https://github.com/netty/netty/pull/2827 addresses this issue

@Scottmitch cherry-picked your change. Thanks!


Pull request review comments:

Patch that resolved the issue:
diff --git a/.gitignore b/.gitignore
index 8a80d67d0d9..822e92dda03 100644
--- a/.gitignore
+++ b/.gitignore
@@ -2,6 +2,7 @@
 .project
 .classpath
 .settings
+bin/
 
 # IntelliJ IDEA project files and directories
 *.iml


Test patch: