netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (1)

145-153: Bug: writes 'n' instead of newline; should be '\n'.

The comment and behavior diverge; replace with newline byte 0x0A.

Apply within this block:

-                    // Replace "\\n" with "\n" (1 byte: actual newline)
-                    buffer.setByte(writePos, (byte) 'n');
+                    // Replace "\\n" with "\n" (1 byte: actual newline)
+                    buffer.setByte(writePos, (byte) '\n');

netty-socketio-core/src/test/java/com/corundumstudio/socketio/benchmark/PreprocessJsonBenchmark.java (2)

109-115: Critical: Memory leak from unreleased ByteBuf instances.

Each benchmark iteration creates a new ByteBuf (via content.copy() in preprocessJson) that is returned but never released. JMH will accumulate these leaked buffers across thousands of iterations, causing memory exhaustion.

Additionally, the past review comment about resetting the buffer's reader index remains unaddressed. While not strictly necessary given that preprocessJson copies the buffer, resetting is defensive practice for benchmarks.

Apply this diff to fix both issues:

 @Benchmark
 public ByteBuf benchmarkNewMethod() throws UnsupportedEncodingException {
     ByteBuf buffer = testBuffers[bufferIndex % testBuffers.length];
+    buffer.readerIndex(0);
     ByteBuf result = decoder.preprocessJson(1, buffer);
     bufferIndex++;
+    result.release();
-    return result;
+    return Unpooled.EMPTY_BUFFER;
 }

Note: Returning Unpooled.EMPTY_BUFFER prevents JMH from optimizing away the method call while avoiding the leak. Alternatively, use JMH's Blackhole to consume the result:

 @Benchmark
-public ByteBuf benchmarkNewMethod() throws UnsupportedEncodingException {
+public void benchmarkNewMethod(Blackhole blackhole) throws UnsupportedEncodingException {
     ByteBuf buffer = testBuffers[bufferIndex % testBuffers.length];
+    buffer.readerIndex(0);
     ByteBuf result = decoder.preprocessJson(1, buffer);
     bufferIndex++;
+    blackhole.consume(result);
+    result.release();
-    return result;
 }

---

117-123: Critical: Memory leak from unreleased ByteBuf instances.

Each benchmark iteration creates a new ByteBuf (via Unpooled.wrappedBuffer() in preprocessJsonOld) that is returned but never released. This causes the same memory leak as benchmarkNewMethod.

Additionally, the past review comment about resetting the buffer's reader index remains unaddressed.

Apply this diff to fix both issues:

 @Benchmark
 public ByteBuf benchmarkOldMethod() throws UnsupportedEncodingException {
     ByteBuf buffer = testBuffers[bufferIndex % testBuffers.length];
+    buffer.readerIndex(0);
     ByteBuf result = preprocessJsonOld(1, buffer);
     bufferIndex++;
+    result.release();
-    return result;
+    return Unpooled.EMPTY_BUFFER;
 }

Or use JMH's Blackhole:

 @Benchmark
-public ByteBuf benchmarkOldMethod() throws UnsupportedEncodingException {
+public void benchmarkOldMethod(Blackhole blackhole) throws UnsupportedEncodingException {
     ByteBuf buffer = testBuffers[bufferIndex % testBuffers.length];
+    buffer.readerIndex(0);
     ByteBuf result = preprocessJsonOld(1, buffer);
     bufferIndex++;
+    blackhole.consume(result);
+    result.release();
-    return result;
 }

netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketDecoderTest.java (1)

569-569: Remove debug print statement.

This debug print statement should be removed from production test code, as noted in the previous review.

Apply this diff:

-            System.out.println(testCase);

.github/workflows/build.yml (1)

26-26: Switch to verify looks good; consider -ntp to cut CI noise.

Keeps semantics while enabling verify-bound plugins. Optional: add -ntp to reduce transfer logs in CI.

Apply within this line:

-          mvn --batch-mode --errors --fail-at-end verify
+          mvn --batch-mode --errors --fail-at-end -ntp verify

netty-socketio-smoke-test/pom.xml (1)

117-124: Checkstyle plugin is added but inert (skip=true, no executions). Intentional?

As configured, it won’t run nor fail the build. If the goal is future enforcement, consider centralizing plugin config in parent POM and binding an execution with <goal>check</goal> (kept skipped by default).

netty-socketio-quarkus/netty-socketio-quarkus-deployment/pom.xml (1)

46-61: Use check goal (fail build) instead of checkstyle (report only) for verify-time enforcement.

Currently, even if you flip <skip>false</skip>, checkstyle won’t fail the build. Use check to enforce.

Apply within these lines:

         <executions>
           <execution>
             <phase>verify</phase>
             <goals>
-              <goal>checkstyle</goal>
+              <goal>check</goal>
             </goals>
           </execution>
         </executions>
         <configuration>
           <skip>true</skip>
         </configuration>

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (1)

89-127: URL decoding behavior changes vs URLDecoder: incomplete % sequences and exception type.

• Incomplete or malformed escapes (e.g., trailing % or %GZ) are passed through, whereas URLDecoder throws. This may alter behavior. Confirm desired compatibility and update tests accordingly.
• UnsupportedEncodingException is not actually thrown; remove from the private method signature.

Apply signature change here:

-    private void urlDecodeInPlace(ByteBuf buffer) throws UnsupportedEncodingException {
+    private void urlDecodeInPlace(ByteBuf buffer) {

If strict parity with URLDecoder.decode(..., UTF-8) is required, detect incomplete % patterns and either throw or replace with U+FFFD per project policy.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketDecoderTest.java (1)

674-699: Remove or downgrade logging statement from production test code.

Line 677 uses log.info() which will clutter test output. Change to log.debug() or remove entirely, as JUnit already tracks test execution.

Apply this diff:

-        log.info("Running preprocessJson test for case: {}", testCase);
+        log.debug("Running preprocessJson test for case: {}", testCase);

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (2)

52-64: Not actually zero-copy; copy() defeats the PR's goal and contradicts Javadoc.

content.copy() at line 63 allocates and memcpy's the entire buffer, which contradicts both the "True zero-copy" claim in the Javadoc and the PR's performance objectives. Use retainedSlice() (or retainedDuplicate()) to achieve actual zero-copy by working on a view of the original buffer with independent indices. The caller must then release the returned buffer.

Apply this diff:

     /**
-     * True zero-copy optimized version of preprocessJson that works directly with ByteBuf
-     * without string conversion and without creating new ByteBuf instances.
-     * 
+     * Zero-copy preprocessing that operates on a retained slice of the input ByteBuf
+     * without string conversion and without allocating new byte arrays.
+     *
      * @param jsonIndex JSONP index, if null then no JSONP processing is needed
      * @param content the input ByteBuf containing the packet data
-     * @return processed ByteBuf with true zero-copy optimization
-     * @throws UnsupportedEncodingException if UTF-8 encoding is not supported
+     * @return processed ByteBuf (caller must release)
      */
     public ByteBuf preprocessJson(Integer jsonIndex, ByteBuf content) throws UnsupportedEncodingException {
-        // Create a mutable copy of the input ByteBuf for in-place modifications
-        ByteBuf mutableContent = content.copy();
+        // Work on a retained slice to avoid copying; caller must release the returned buffer
+        ByteBuf mutableContent = content.retainedSlice();

---

69-77: Guard skipping of the "d=" prefix; current code drops first 2 bytes unconditionally when jsonIndex != null.

This can corrupt payloads that don't start with d=. Verify the prefix before advancing the reader index.

Apply this diff:

-                // Skip "d=" prefix (2 bytes) by adjusting reader index
+                // Skip "d=" prefix (2 bytes) only if present
                 if (mutableContent.readableBytes() >= 2) {
-                    mutableContent.readerIndex(mutableContent.readerIndex() + 2);
+                    int ri = mutableContent.readerIndex();
+                    if (mutableContent.getByte(ri) == (byte) 'd'
+                            && mutableContent.getByte(ri + 1) == (byte) '=') {
+                        mutableContent.readerIndex(ri + 2);
+                    }
                 }

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (1)

130-154: Clarify confusing comment notation for escaped newline replacement.

The comment at line 130 uses identical notation for both the before and after states: "\\n" with "\n", making it unclear what transformation is being described. The code correctly replaces a 3-byte sequence (backslash, backslash, n) with a 2-byte sequence (backslash, n), matching the legacy implementation at line 711 in PacketDecoderTest.java. Clarify the comment to explicitly state the byte-level transformation to avoid confusion.

Consider this clarification:

     /**
-     * Replace escaped newlines "\\n" with "\n" in-place
-     * Note: This reduces the buffer size from 3 bytes("\\n") to 2 byte("\n") per replacement
+     * Replace double-escaped newlines with single-escaped newlines in-place.
+     * Transforms the 3-byte sequence (backslash, backslash, 'n') to 2-byte sequence (backslash, 'n').
      * because unescaping of new lines can be done safely on server-side(c) socket.io.js
      * @see https://github.com/Automattic/socket.io-client/blob/1.3.3/socket.io.js#L2682
      */

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (2)

139-179: Escaped newline replacement is functionally wrong; write '\n' and match 2-byte pattern (“\” + “n”).

• Should detect backslash + 'n' (two bytes), not “\n” (three).
• Should write the newline byte 0x0A, not '' and 'n'.
• Size reduces 2 bytes → 1 byte, not “3 → 2”.

-    /**
-     * Replace escaped newlines "\\n" with "\n" in-place
-     * Note: This reduces the buffer size from 3 bytes("\\n") to 2 byte("\n") per replacement
+    /**
+     * Replace escaped newlines "\\n" with a real newline in-place.
+     * Note: This reduces the buffer size from 2 bytes ("\\n") to 1 byte ('\n') per replacement
      * because unescaping of new lines can be done safely on server-side(c) socket.io.js
      * @see https://github.com/Automattic/socket.io-client/blob/1.3.3/socket.io.js#L2682
      */
     private void replaceEscapedNewlinesInPlace(ByteBuf buffer) {
         int readerIndex = buffer.readerIndex();
         int writerIndex = buffer.writerIndex();
         int readPos = readerIndex;
         int writePos = readerIndex;
         
         while (readPos < writerIndex) {
             byte b = buffer.getByte(readPos);
             
-            // Check for "\\\\n" pattern (real 3 bytes: "\\n")
-            if (b == '\\' && readPos + 2 < writerIndex) {
-                byte b1 = buffer.getByte(readPos + 1);
-                byte b2 = buffer.getByte(readPos + 2);
-
-                if (b1 == '\\' && b2 == 'n') {
-                    buffer.setByte(writePos, (byte) '\\');
-                    writePos++;
-                    buffer.setByte(writePos, (byte) 'n');
-                    writePos++;
-                    readPos += 3; // Skip both bytes
+            // Check for "\n" escape (two bytes: '\' + 'n')
+            if (b == '\\' && readPos + 1 < writerIndex) {
+                byte b1 = buffer.getByte(readPos + 1);
+                if (b1 == 'n') {
+                    buffer.setByte(writePos, (byte) '\n');
+                    writePos++;
+                    readPos += 2; // consume '\' and 'n'
                 } else {
                     buffer.setByte(writePos, b);
                     writePos++;
                     readPos++;
                 }
             } else {
                 buffer.setByte(writePos, b);
                 writePos++;
                 readPos++;
             }
         }
         
         // Adjust writer index to reflect the new length
         buffer.writerIndex(writePos);
     }

---

66-93: Zero-copy semantics + refCnt safety: avoid slice()+release(); mutate in place or retain the slice.

Current flow:

• slice() returns a derived buffer sharing refCnt with content.
• releasing the slice in catch can prematurely release the original.
• Returning an unretained slice can cause lifetime confusion.

Prefer true in-place mutation and return the same buffer, or retain the slice and never release here.

Option A (simplest, true zero-copy): mutate input and return it.

-        // Create a mutable copy of the input ByteBuf for in-place modifications
-        ByteBuf mutableContent = content.slice();
-        try {
-            // Perform URL decoding in-place
-            urlDecodeInPlace(mutableContent);
-            
-            if (jsonIndex != null) {
-                // Handle escaped newlines in-place: replace "\\n" with "\n"
-                replaceEscapedNewlinesInPlace(mutableContent);
-                
-                // Skip "d=" prefix (2 bytes) by adjusting reader index
-                if (mutableContent.readableBytes() >= 2) {
-                    int ri = mutableContent.readerIndex();
-                    // Check for 'd=' prefix
-                    if (mutableContent.getByte(ri) == (byte) 'd'
-                            && mutableContent.getByte(ri + 1) == (byte) '=') {
-                        mutableContent.readerIndex(ri + 2);
-                    } else {
-                        throw new IllegalArgumentException("Invalid JSONP format: missing 'd=' prefix");
-                    }
-                }
-            }
-            
-            return mutableContent;
-        } catch (Exception e) {
-            mutableContent.release();
-            throw e;
-        }
+        // Mutate content in place; no allocation, no refCnt juggling
+        urlDecodeInPlace(content);
+
+        if (jsonIndex != null) {
+            // Replace "\n" escape with newline and then skip "d="
+            replaceEscapedNewlinesInPlace(content);
+
+            if (content.readableBytes() >= 2) {
+                int ri = content.readerIndex();
+                if (content.getByte(ri) == (byte) 'd' && content.getByte(ri + 1) == (byte) '=') {
+                    content.readerIndex(ri + 2);
+                } else {
+                    throw new IllegalArgumentException("Invalid JSONP format: missing 'd=' prefix");
+                }
+            }
+        }
+        return content;

Option B (if you need independent indices): use retainedSlice() and don’t release here; document caller must release the returned buffer.

-        ByteBuf mutableContent = content.slice();
+        ByteBuf mutableContent = content.retainedSlice();
         try {
             urlDecodeInPlace(mutableContent);
             ...
             return mutableContent;
         } catch (Exception e) {
-            mutableContent.release();
+            // no release of 'content' here; only the retained slice
+            mutableContent.release();
             throw e;
         }

netty-socketio-core/src/main/java/com/corundumstudio/socketio/protocol/PacketDecoder.java (2)

56-64: Javadoc overstates “true zero-copy” and “no new ByteBuf instances.”

You create a derived buffer via slice(). Clarify semantics (zero-copy: no new byte arrays; may return the same buffer or a slice).

-    /**
-     * True zero-copy optimized version of preprocessJson that works directly with ByteBuf
-     * without string conversion and without creating new ByteBuf instances.
-     * 
+    /**
+     * Zero-copy preprocessing that operates directly on a ByteBuf without string conversion
+     * and without allocating new byte arrays. It mutates the buffer contents in place and
+     * returns a buffer view of the processed data.
+     *

---

99-137: No need to declare UnsupportedEncodingException; consider removing redundant throws.

urlDecodeInPlace does not use APIs that can throw UnsupportedEncodingException. Consider removing it here and from preprocessJson’s signature (if source/binary compatibility allows).

netty-socketio-core/src/test/java/com/corundumstudio/socketio/protocol/PacketDecoderTest.java (2)

674-698: Release newResult and reduce test log noise.

Add release to avoid leaks if preprocessJson returns a retained slice; prefer debug level for noisy per‑case logs.

-        log.info("Running preprocessJson test for case: {}", testCase);
+        log.debug("Running preprocessJson test for case: {}", testCase);
@@
-        // Clean up
-        buffer.release();
-        originalResult.release();
+        // Clean up
+        buffer.release();
+        originalResult.release();
+        if (newResult.refCnt() > 0) {
+            newResult.release();
+        }

If you keep returning the original buffer (in‑place), the added release will be a no‑op due to shared refCnt; if you switch to retainedSlice(), this prevents leaks. Please confirm with a quick CI run.

---

498-671: Large combinatorial inputs may slow CI; consider tagging as “slow” or parameterizing.

The exhaustive encoded cases are great for coverage but can make unit tests heavy. Consider moving them under a separate @tag("slow") or splitting into parameterized subsets.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro