feedback-to-code/mozzarella huggingface dataset:
Repository: netty/netty
Issue: Log4J2 Auto-detection

Not sure if I'm missing something...

[#5047](https://github.com/netty/netty/pull/5047) added Log4J2LoggerFactory and Log4J2Logger. It appears that Log4J2 auto-detection in InternalLoggerFactory wasn't added?
https://github.com/netty/netty/blob/fef462c04333950201ddbfd505c0af730a0c2ffd/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java#L39

Cheers!


Issue comments:
@apimastery good catch... See https://github.com/netty/netty/pull/8109

Pull request review comments:

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java b/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java
index 9f85e3646b4..12c1b5a4477 100644
--- a/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java
+++ b/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java
@@ -41,13 +41,18 @@ private static InternalLoggerFactory newDefaultFactory(String name) {
         try {
             f = new Slf4JLoggerFactory(true);
             f.newInstance(name).debug("Using SLF4J as the default logging framework");
-        } catch (Throwable t1) {
+        } catch (Throwable ignore1) {
             try {
                 f = Log4JLoggerFactory.INSTANCE;
                 f.newInstance(name).debug("Using Log4J as the default logging framework");
-            } catch (Throwable t2) {
-                f = JdkLoggerFactory.INSTANCE;
-                f.newInstance(name).debug("Using java.util.logging as the default logging framework");
+            } catch (Throwable ignore2) {
+                try {
+                    f = Log4J2LoggerFactory.INSTANCE;
+                    f.newInstance(name).debug("Using Log4J2 as the default logging framework");
+                } catch (Throwable ignore3) {
+                    f = JdkLoggerFactory.INSTANCE;
+                    f.newInstance(name).debug("Using java.util.logging as the default logging framework");
+                }
             }
         }
         return f;


Test patch:
Repository: netty/netty
Issue: Only apply copyMemoy safepoint for java <= 8
Labels: cleanup

Hi, as per https://bugs.openjdk.java.net/browse/JDK-8149596 (and the source code of Java.nio.Bits) safepoint polling is no longer required (or rather it shouldn't be done). It's included as part of Unsafe.copyMemory.

Can we add a check to PlatformDependent where this occurs for java version <= 8 so we don't essentially do safepoint polling twice?

Thanks.

Issue comments:
sounds good.
Thanks for the PR. Looks good.
b109, should we be checking build too?
@johnou honestly I think we either should either just assume its ok if people use java8 (and if its not they should really upgrade) or only not use it for java9 and higher. /cc @hc-codersatlas WDYT ?
@normanmaurer agree

Pull request review comments:
Not a part of this commit, but is this commented out code necessary? Same above.
Optional: consider splitting the manual safepointer to make the inliner happy.

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/internal/PlatformDependent0.java b/common/src/main/java/io/netty/util/internal/PlatformDependent0.java
index c6ebb55131d..53e9ed7bdcf 100644
--- a/common/src/main/java/io/netty/util/internal/PlatformDependent0.java
+++ b/common/src/main/java/io/netty/util/internal/PlatformDependent0.java
@@ -550,7 +550,16 @@ static void putLong(byte[] data, int index, long value) {
     }
 
     static void copyMemory(long srcAddr, long dstAddr, long length) {
-        //UNSAFE.copyMemory(srcAddr, dstAddr, length);
+        // Manual safe-point polling is only needed prior Java9:
+        // See https://bugs.openjdk.java.net/browse/JDK-8149596
+        if (javaVersion() <= 8) {
+            copyMemoryWithSafePointPolling(srcAddr, dstAddr, length);
+        } else {
+            UNSAFE.copyMemory(srcAddr, dstAddr, length);
+        }
+    }
+
+    private static void copyMemoryWithSafePointPolling(long srcAddr, long dstAddr, long length) {
         while (length > 0) {
             long size = Math.min(length, UNSAFE_COPY_THRESHOLD);
             UNSAFE.copyMemory(srcAddr, dstAddr, size);
@@ -561,7 +570,17 @@ static void copyMemory(long srcAddr, long dstAddr, long length) {
     }
 
     static void copyMemory(Object src, long srcOffset, Object dst, long dstOffset, long length) {
-        //UNSAFE.copyMemory(src, srcOffset, dst, dstOffset, length);
+        // Manual safe-point polling is only needed prior Java9:
+        // See https://bugs.openjdk.java.net/browse/JDK-8149596
+        if (javaVersion() <= 8) {
+            copyMemoryWithSafePointPolling(src, srcOffset, dst, dstOffset, length);
+        } else {
+            UNSAFE.copyMemory(src, srcOffset, dst, dstOffset, length);
+        }
+    }
+
+    private static void copyMemoryWithSafePointPolling(
+            Object src, long srcOffset, Object dst, long dstOffset, long length) {
         while (length > 0) {
             long size = Math.min(length, UNSAFE_COPY_THRESHOLD);
             UNSAFE.copyMemory(src, srcOffset, dst, dstOffset, size);


Test patch:
Repository: netty/netty
Issue: Infinite loop in JdkZlibEncoder.deflate()

<pre>
private void deflate(ByteBuf out) {
    int numBytes;
    do {
        int writerIndex = out.writerIndex();
        numBytes = deflater.deflate(
                out.array(), out.arrayOffset() + writerIndex, out.writableBytes(), Deflater.SYNC_FLUSH);
        out.writerIndex(writerIndex + numBytes);
    } while (numBytes > 0);
}
</pre>
if out.writableBytes() is ZERO and numBytes>0, the function will never return!

Issue comments:
Are you interested in providing a PR?
@swrdlgc 
@swrdlgc actually this can never happen as `deflate(...)` will return the number of bytes written to the output buffer which will be 0 if it was not writable.
The good thing is because of this bug report I think I spotted a bug which is unrelated to the reported problem tho ;)
thanks for replay, you are right, this can never happen.


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 f039fa66e84..276d7f86b0c 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
@@ -225,8 +225,18 @@ protected void encode(ChannelHandlerContext ctx, ByteBuf uncompressed, ByteBuf o
         }
 
         deflater.setInput(inAry, offset, len);
-        while (!deflater.needsInput()) {
+        for (;;) {
             deflate(out);
+            if (deflater.needsInput()) {
+                // Consumed everything
+                break;
+            } else {
+                if (!out.isWritable()) {
+                    // We did not consume everything but the buffer is not writable anymore. Increase the capacity to
+                    // make more room.
+                    out.ensureWritable(out.writerIndex());
+                }
+            }
         }
     }
 


Test patch:
Repository: netty/netty
Issue: In case of connection issue channel.remoteAddress() returns null

### Expected behavior

It should return the request address, which is correctly passed into the `AnnotatedConnectionException`:

`
        protected final Throwable annotateConnectException(Throwable cause, SocketAddress remoteAddress) {
...
}
`

### Actual behavior

It returns `null` when trying to get it from the channel on receiving an exception.

### Steps to reproduce

### Minimal yet complete reproducer code (or URL to code)

### Netty version

4.x.x.

### JVM version (e.g. `java -version`)

### OS version (e.g. `uname -a`)


Issue comments:
This is not actually a bug, because the spec states that the method returns `null` if the channel is not connected:

> {@code null} if this channel is not connected.

But could be an improvement because it is useful to have this information in some error handlers.
Agree. If `channelInactive` cannot see remote address, how could it reconnect?

Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
index 01467b98361..74f798e7aca 100644
--- a/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
+++ b/transport/src/main/java/io/netty/channel/nio/AbstractNioChannel.java
@@ -515,4 +515,13 @@ protected void doClose() throws Exception {
             connectTimeoutFuture = null;
         }
     }
+
+    @Override
+    public SocketAddress remoteAddress() {
+        final SocketAddress remoteAddress = super.remoteAddress();
+        if (remoteAddress == null) {
+            return requestedRemoteAddress;
+        }
+        return remoteAddress;
+    }
 }


Test patch:
Repository: netty/netty
Issue: Add logs for allocating large bytebuf in PooledByteBufAllocator

### Expected behavior
Print logs for allocating large bytebuf in PooledByteBufAllocator, because many applications may care about the memory consumption, and want's to know more info about the memory allocation in case of OOM(for instance, Direct Memory OOM). 
### Actual behavior
No logs
### Steps to reproduce
NA
### Minimal yet complete reproducer code (or URL to code)
```
  protected ByteBuf newDirectBuffer(int initialCapacity, int maxCapacity) {
        PoolThreadCache cache = threadCache.get();
        PoolArena<ByteBuffer> directArena = cache.directArena;

        final ByteBuf buf;
        if (directArena != null) {
            buf = directArena.allocate(cache, initialCapacity, maxCapacity);
        } else {
            buf = PlatformDependent.hasUnsafe() ?
                    UnsafeByteBufUtil.newUnsafeDirectByteBuf(this, initialCapacity, maxCapacity) :
                    new UnpooledDirectByteBuf(this, initialCapacity, maxCapacity);
        }

        return toLeakAwareBuffer(buf);
    }
```
### Netty version
4.1.30

### JVM version (e.g. `java -version`)
NA
### OS version (e.g. `uname -a`)
NA


Issue comments:

Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java b/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java
index de6eee1d54d..440b42cef3c 100644
--- a/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java
+++ b/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java
@@ -307,6 +307,11 @@ private static int validateAndCalculateChunkSize(int pageSize, int maxOrder) {
 
     @Override
     protected ByteBuf newHeapBuffer(int initialCapacity, int maxCapacity) {
+        if (initialCapacity > 1024 * 1024) {
+            logger.info("Try to allocate heap buffer with initialCapacity: " +
+                initialCapacity + " maxCapacity: " + maxCapacity + ", current usedHeapMemory: " +
+                metric.usedHeapMemory());
+        }
         PoolThreadCache cache = threadCache.get();
         PoolArena<byte[]> heapArena = cache.heapArena;
 
@@ -324,6 +329,11 @@ protected ByteBuf newHeapBuffer(int initialCapacity, int maxCapacity) {
 
     @Override
     protected ByteBuf newDirectBuffer(int initialCapacity, int maxCapacity) {
+        if (initialCapacity > 1024 * 1024) {
+            logger.info("Try to allocate direct buffer with initialCapacity: " +
+                initialCapacity + " maxCapacity: " + maxCapacity + ", current usedDirectMemory: " +
+                metric.usedDirectMemory());
+        }
         PoolThreadCache cache = threadCache.get();
         PoolArena<ByteBuffer> directArena = cache.directArena;
 


Test patch:
Repository: netty/netty
Issue: Typo Documentation - MessageSizeEstimator.class

https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/transport/src/main/java/io/netty/channel/MessageSizeEstimator.java#L19

"Responsible to estimate size of a message. The size represent how much memory the message will ca. reserve in memory."

I was going through the source and trying to determine where MessageSizeEstimator.class is used and what "ca" in the documentation means. I will make the pull request once I know what "ca" means, if it needs to be corrected.




Issue comments:
@Mr00Anderson ca. == approximately  . So yep PRs welcome 

Pull request review comments:

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/MessageSizeEstimator.java b/transport/src/main/java/io/netty/channel/MessageSizeEstimator.java
index 5c84927781a..92f164dc5d5 100644
--- a/transport/src/main/java/io/netty/channel/MessageSizeEstimator.java
+++ b/transport/src/main/java/io/netty/channel/MessageSizeEstimator.java
@@ -16,8 +16,8 @@
 package io.netty.channel;
 
 /**
- * Responsible to estimate size of a message. The size represent how much memory the message will ca. reserve in
- * memory.
+ * Responsible to estimate the size of a message. The size represents approximately how much memory the message will
+ * reserve in memory.
  */
 public interface MessageSizeEstimator {
 


Test patch:
Repository: netty/netty
Issue: readComplete when handlerRemoved

### Expected behavior
dont call fireChannelReadComplete
call fireHandlerRemoved or nothing

### Actual behavior
call fireChannelReadComplete

https://github.com/netty/netty/blob/a33200ca38990b88315a48637e5ac5da398b100d/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
line 255
>ctx.fireChannelReadComplete();

### Steps to reproduce
handler is removed in port unification
https://netty.io/4.1/xref/io/netty/example/portunification/PortUnificationServerHandler.html
>p.remove(this);

the removing handler has nothing to do with readComplete.

### Minimal yet complete reproducer code (or URL to code)
you can reproduce it anytime by removing handler.

### Netty version
4.1.31

### JVM version (e.g. `java -version`)
corretto 8

### OS version (e.g. `uname -a`)
win 10

Issue comments:
There is no `fireHandlerXXX` method in `ChannelHandlerContext`, only `fireChannelXXX` method. If you want to propagate handler Removed event, you can try `fireUserEventTriggered`.


My understanding of channelReadComplete () may not be sufficient.

If it is a valid specification that channelReadComplete () is called when deleting a handler in port unification, close this issue.
The behavior was unexpected just for me.
Thanks @lifeinwild, I think you're right that it would be better for the removal not to trigger this unless the handler is mid-read. I see that @normanmaurer has opened #9211 to address it.
Yes I did fix it... closing

Pull request review comments:

Patch that resolved the issue:
diff --git a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
index bed1efc211c..0457a93abdc 100644
--- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
+++ b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java
@@ -241,18 +241,16 @@ public final void handlerRemoved(ChannelHandlerContext ctx) throws Exception {
         if (buf != null) {
             // Directly set this to null so we are sure we not access it in any other method here anymore.
             cumulation = null;
-
+            numReads = 0;
             int readable = buf.readableBytes();
             if (readable > 0) {
                 ByteBuf bytes = buf.readBytes(readable);
                 buf.release();
                 ctx.fireChannelRead(bytes);
+                ctx.fireChannelReadComplete();
             } else {
                 buf.release();
             }
-
-            numReads = 0;
-            ctx.fireChannelReadComplete();
         }
         handlerRemoved0(ctx);
     }


Test patch:
Repository: netty/netty
Issue: ByteBufInputStream.read() may have a problem

i use netty4.1.34

souce code:

```
public int read() throws IOException {
        if (!buffer.isReadable()) {
            return -1;
        }
        return buffer.readByte() & 0xff;
    }
```
In my opinion, the buffer.isReadable() method should not be used here to judge, but the available() method should be used. Because ByteBufInputStream is passed in length when constructing, so if the buffer.isReadable() method is used here If judged, it may exceed the limit of length, which is unreasonable.

Issue comments:
@xiaoheng1 good point... are you willing to open a PR ?
@normanmaurer I am glad to，i will open a PR.
#9306 Fix public int read() throws IOException method exceeds the limit of length

Pull request review comments:
nit: merge both lines 
Call buf.release() and in.close()
Call buf2.release() and in2.close()
@normanmaurer ok, i add it.

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/ByteBufInputStream.java b/buffer/src/main/java/io/netty/buffer/ByteBufInputStream.java
index 038cd8db459..c0b116c1483 100644
--- a/buffer/src/main/java/io/netty/buffer/ByteBufInputStream.java
+++ b/buffer/src/main/java/io/netty/buffer/ByteBufInputStream.java
@@ -163,7 +163,8 @@ public boolean markSupported() {
 
     @Override
     public int read() throws IOException {
-        if (!buffer.isReadable()) {
+        int available = available();
+        if (available == 0) {
             return -1;
         }
         return buffer.readByte() & 0xff;


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/ByteBufStreamTest.java b/buffer/src/test/java/io/netty/buffer/ByteBufStreamTest.java
index 222ddcf7ee7..989e160b22a 100644
--- a/buffer/src/test/java/io/netty/buffer/ByteBufStreamTest.java
+++ b/buffer/src/test/java/io/netty/buffer/ByteBufStreamTest.java
@@ -212,4 +212,39 @@ public void testReadLine() throws Exception {
         assertEquals(charCount, count);
         in.close();
     }
+
+    @Test
+    public void testRead() throws Exception {
+        // case1
+        ByteBuf buf = Unpooled.buffer(16);
+        buf.writeBytes(new byte[]{1, 2, 3, 4, 5, 6});
+
+        ByteBufInputStream in = new ByteBufInputStream(buf, 3);
+
+        assertEquals(1, in.read());
+        assertEquals(2, in.read());
+        assertEquals(3, in.read());
+        assertEquals(-1, in.read());
+        assertEquals(-1, in.read());
+        assertEquals(-1, in.read());
+
+        buf.release();
+        in.close();
+
+        // case2
+        ByteBuf buf2 = Unpooled.buffer(16);
+        buf2.writeBytes(new byte[]{1, 2, 3, 4, 5, 6});
+
+        ByteBufInputStream in2 = new ByteBufInputStream(buf2, 4);
+
+        assertEquals(1, in2.read());
+        assertEquals(2, in2.read());
+        assertEquals(3, in2.read());
+        assertEquals(4, in2.read());
+        assertNotEquals(5, in2.read());
+        assertEquals(-1, in2.read());
+
+        buf2.release();
+        in2.close();
+    }
 }
Repository: netty/netty
Issue: A redundant comparison operation

Hi, when I read the source code, I happened to notice this
https://github.com/netty/netty/blob/ff2a7929235257642d8f63edb1c580347307f3ee/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java#L669-L674
The `eventLoop == null` is unnecessary. Although the c2 compiler optimizes it, c1 doesn’t seem to optimize it. Though it’s a small problem，it doesn’t look elegant enough.

equivalent
```java
if (eventLoop == this) {
    unsafe.close(unsafe.voidPromise());
}
return;
```

Issue comments:
processSelectedKey should be an hot path and very likely to be compiled at C2 level so I don't think it would make any difference perf-wise. For readability is fine, agree :)
@MrYangxf sounds good... Can you please submit a PR ?
@normanmaurer Of course :)

Pull request review comments:
this `return` can be removed now. 
this seems to violate the original logic. The healthy channel  may be closed when `eventLoop != this`
@MrYangxf not sure I understand your concern... With your change `return` is the last statement in the method which makes it redundant.. What I am missing ?
@normanmaurer  ...`return` is contained in the `if(!k.isValid())` statement instead of the last line of the method...
@MrYangxf doh! stupid me ... I did not notice that there is another scope. You are right, of course. 

Patch that resolved the issue:
diff --git a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
index ace430afc95..36a0b76ddc0 100644
--- a/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
+++ b/transport/src/main/java/io/netty/channel/nio/NioEventLoop.java
@@ -666,11 +666,10 @@ private void processSelectedKey(SelectionKey k, AbstractNioChannel ch) {
             // and thus the SelectionKey could be cancelled as part of the deregistration process, but the channel is
             // still healthy and should not be closed.
             // See https://github.com/netty/netty/issues/5125
-            if (eventLoop != this || eventLoop == null) {
-                return;
+            if (eventLoop == this) {
+                // close the channel if the key is not valid anymore
+                unsafe.close(unsafe.voidPromise());
             }
-            // close the channel if the key is not valid anymore
-            unsafe.close(unsafe.voidPromise());
             return;
         }
 


Test patch:
Repository: netty/netty
Issue: Incorrect license id in NOTICE.txt

The NOTICE.txt file ends with:

```
This product contains the dnsinfo.h header file, that provides a way to retrieve the system DNS configuration on MacOS.
This private header is also used by Apple's open source
 mDNSResponder (https://opensource.apple.com/tarballs/mDNSResponder/).

 * LICENSE:
    * license/LICENSE.dnsinfo.txt (Apache License 2.0)
  * HOMEPAGE:
    * https://www.opensource.apple.com/source/configd/configd-453.19/dnsinfo/dnsinfo.h
```

This license is the Apple Public Source License 2.0, not the Apache License 2.0.

---

Issue comments:
Thanks for reporting. #10773 PR will fix this.
@hyandell doh! Thanks for catching this.

Pull request review comments:

Patch that resolved the issue:
diff --git a/NOTICE.txt b/NOTICE.txt
index b702bb72403..cd84a8afe20 100644
--- a/NOTICE.txt
+++ b/NOTICE.txt
@@ -243,6 +243,6 @@ This private header is also used by Apple's open source
  mDNSResponder (https://opensource.apple.com/tarballs/mDNSResponder/).
 
  * LICENSE:
-    * license/LICENSE.dnsinfo.txt (Apache License 2.0)
+    * license/LICENSE.dnsinfo.txt (Apple Public Source License 2.0)
   * HOMEPAGE:
-    * https://www.opensource.apple.com/source/configd/configd-453.19/dnsinfo/dnsinfo.h
\ No newline at end of file
+    * https://www.opensource.apple.com/source/configd/configd-453.19/dnsinfo/dnsinfo.h


Test patch: