From 501c35afff01be0e06943b9fffb87fad1838ce5a Mon Sep 17 00:00:00 2001
From: Xiaoyan Lin <linxiaoyan18@gmail.com>
Date: Thu, 28 Jan 2016 11:25:54 -0800
Subject: [PATCH] Retain ByteBuf extras when aggregating

Motivation:

BinaryMemcacheObjectAggregator doesn't retain ByteBuf `extras`. So `io.netty.util.IllegalReferenceCountException: refCnt: 0, decrement: 1` will be thrown when aggregating a message containing `extras`. See the unit test for an example.

Modifications:

`ratain` extras to fix IllegalReferenceCountException.

Result:

`extras` is retained.
---
 .../BinaryMemcacheObjectAggregator.java       |  6 ++-
 .../BinaryMemcacheObjectAggregatorTest.java   | 41 +++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java
index b07db33c4c..65b099468b 100644
--- a/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java
+++ b/codec-memcache/src/main/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregator.java
@@ -53,8 +53,9 @@ public class BinaryMemcacheObjectAggregator extends AbstractMemcacheObjectAggreg
     }
 
     private static FullBinaryMemcacheRequest toFullRequest(BinaryMemcacheRequest request, ByteBuf content) {
+        ByteBuf extras = request.extras() == null ? null : request.extras().retain();
         FullBinaryMemcacheRequest fullRequest =
-                new DefaultFullBinaryMemcacheRequest(request.key(), request.extras(), content);
+                new DefaultFullBinaryMemcacheRequest(request.key(), extras, content);
 
         fullRequest.setMagic(request.magic());
         fullRequest.setOpcode(request.opcode());
@@ -70,8 +71,9 @@ public class BinaryMemcacheObjectAggregator extends AbstractMemcacheObjectAggreg
     }
 
     private static FullBinaryMemcacheResponse toFullResponse(BinaryMemcacheResponse response, ByteBuf content) {
+        ByteBuf extras = response.extras() == null ? null : response.extras().retain();
         FullBinaryMemcacheResponse fullResponse =
-                new DefaultFullBinaryMemcacheResponse(response.key(), response.extras(), content);
+                new DefaultFullBinaryMemcacheResponse(response.key(), extras, content);
 
         fullResponse.setMagic(response.magic());
         fullResponse.setOpcode(response.opcode());
diff --git a/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregatorTest.java b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregatorTest.java
index e291d61c3e..29e231a1c5 100644
--- a/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregatorTest.java
+++ b/codec-memcache/src/test/java/io/netty/handler/codec/memcache/binary/BinaryMemcacheObjectAggregatorTest.java
@@ -18,12 +18,17 @@ package io.netty.handler.codec.memcache.binary;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.embedded.EmbeddedChannel;
+import io.netty.handler.codec.memcache.DefaultLastMemcacheContent;
+import io.netty.handler.codec.memcache.DefaultMemcacheContent;
+import io.netty.util.CharsetUtil;
 import org.junit.Test;
 
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.*;
 import static org.hamcrest.core.IsNull.notNullValue;
 import static org.hamcrest.core.IsNull.nullValue;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Verifies the correct functionality of the {@link BinaryMemcacheObjectAggregator}.
@@ -73,4 +78,40 @@ public class BinaryMemcacheObjectAggregatorTest {
 
         channel.finish();
     }
+
+    @Test
+    public void shouldRetainByteBufWhenAggregating() {
+        channel = new EmbeddedChannel(
+                new BinaryMemcacheRequestEncoder(),
+                new BinaryMemcacheRequestDecoder(),
+                new BinaryMemcacheObjectAggregator(MAX_CONTENT_SIZE));
+
+        String key = "Netty";
+        ByteBuf extras = Unpooled.copiedBuffer("extras", CharsetUtil.UTF_8);
+        BinaryMemcacheRequest request = new DefaultBinaryMemcacheRequest(key, extras);
+        request.setKeyLength((short) key.length());
+        request.setExtrasLength((byte) extras.readableBytes());
+
+        DefaultMemcacheContent content1 =
+                new DefaultMemcacheContent(Unpooled.copiedBuffer("Netty", CharsetUtil.UTF_8));
+        DefaultLastMemcacheContent content2 =
+                new DefaultLastMemcacheContent(Unpooled.copiedBuffer(" Rocks!", CharsetUtil.UTF_8));
+        int totalBodyLength = key.length() + extras.readableBytes() +
+                content1.content().readableBytes() + content2.content().readableBytes();
+        request.setTotalBodyLength(totalBodyLength);
+
+        assertTrue(channel.writeOutbound(request, content1, content2));
+
+        assertThat(channel.outboundMessages().size(), is(3));
+        assertTrue(channel.writeInbound(channel.readOutbound(), channel.readOutbound(), channel.readOutbound()));
+
+        FullBinaryMemcacheRequest read = channel.readInbound();
+        assertThat(read, notNullValue());
+        assertThat(read.key(), is("Netty"));
+        assertThat(read.extras().toString(CharsetUtil.UTF_8), is("extras"));
+        assertThat(read.content().toString(CharsetUtil.UTF_8), is("Netty Rocks!"));
+
+        read.release();
+        assertFalse(channel.finish());
+    }
 }