andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix memory leak in DefaultCompositeByteBuf when a component is another CompositeByteBuf / Allow retain() and release() on a derived buffer

Browse Source
This commit is contained in:
Trustin Lee 2013-03-14 16:37:20 +09:00
parent 60d9984db1
commit 0f351d2c47
4 changed files with 65 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
buffer/src/main/java/io/netty/buffer/AbstractDerivedByteBuf.java
View File

@ -33,22 +33,24 @@ public abstract class AbstractDerivedByteBuf extends AbstractByteBuf {
@Override @Override
public final ByteBuf retain() { public final ByteBuf retain() {
unwrap().retain();
return this; return this;
} }
@Override @Override
public final ByteBuf retain(int increment) { public final ByteBuf retain(int increment) {
unwrap().retain(increment);
return this; return this;
} }
@Override @Override
public final boolean release() { public final boolean release() {
return false; return unwrap().release();
} }
@Override @Override
public final boolean release(int decrement) { public final boolean release(int decrement) {
return false; return unwrap().release(decrement);
} }
@Override @Override

22
buffer/src/main/java/io/netty/buffer/DefaultCompositeByteBuf.java
View File

@ -228,13 +228,18 @@ public class DefaultCompositeByteBuf extends AbstractReferenceCountedByteBuf
List<Component> list = compositeBuf.components; List<Component> list = compositeBuf.components;
ByteBuf[] array = new ByteBuf[list.size()]; ByteBuf[] array = new ByteBuf[list.size()];
for (int i = 0; i < array.length; i ++) { for (int i = 0; i < array.length; i ++) {
ByteBuf slice = list.get(i).buf; array[i] = list.get(i).buf.retain();
ByteBuf buf;
for (buf = slice; buf.unwrap() != null; buf = buf.unwrap()) {
continue;
} }
buf.retain(); compositeBuf.release();
array[i] = slice; return addComponents0(cIndex, array);
}
if (buffers instanceof CompositeByteBuf) {
CompositeByteBuf compositeBuf = (CompositeByteBuf) buffers;
final int nComponents = compositeBuf.numComponents();
ByteBuf[] array = new ByteBuf[nComponents];
for (int i = 0; i < nComponents; i ++) {
array[i] = compositeBuf.component(i).retain();
} }
compositeBuf.release(); compositeBuf.release();
return addComponents0(cIndex, array); return addComponents0(cIndex, array);
@ -1258,11 +1263,6 @@ public class DefaultCompositeByteBuf extends AbstractReferenceCountedByteBuf
void freeIfNecessary() { void freeIfNecessary() {
// Unwrap so that we can free slices, too. // Unwrap so that we can free slices, too.
ByteBuf buf;
for (buf = this.buf; buf.unwrap() != null; buf = buf.unwrap()) {
continue;
}
if (suspendedDeallocations == null) { if (suspendedDeallocations == null) {
buf.release(); // We should not get a NPE here. If so, it must be a bug. buf.release(); // We should not get a NPE here. If so, it must be a bug.
} else { } else {

5
buffer/src/test/java/io/netty/buffer/AbstractByteBufTest.java
View File

@ -72,13 +72,8 @@ public abstract class AbstractByteBufTest {
@After @After
public void dispose() { public void dispose() {
if (buffer != null) { if (buffer != null) {
if (buffer.unwrap() == null) {
assertThat(buffer.release(), is(true)); assertThat(buffer.release(), is(true));
assertThat(buffer.refCnt(), is(0)); assertThat(buffer.refCnt(), is(0));
} else {
assertThat(buffer.release(), is(false));
assertThat(buffer.refCnt(), is(1));
}
try { try {
buffer.release(); buffer.release();

70
buffer/src/test/java/io/netty/buffer/UnpooledTest.java
View File

@ -16,16 +16,19 @@
package io.netty.buffer; package io.netty.buffer;
import org.easymock.EasyMock; import org.easymock.EasyMock;
import org.junit.After;
import org.junit.Test; import org.junit.Test;
import java.io.InputStream; import java.io.InputStream;
import java.nio.ByteBuffer; import java.nio.ByteBuffer;
import java.nio.channels.ScatteringByteChannel; import java.nio.channels.ScatteringByteChannel;
import java.util.ArrayDeque;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.LinkedHashMap; import java.util.LinkedHashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Map.Entry; import java.util.Map.Entry;
import java.util.Queue;
import static io.netty.buffer.Unpooled.*; import static io.netty.buffer.Unpooled.*;
import static org.junit.Assert.*; import static org.junit.Assert.*;
@ -36,6 +39,27 @@ import static org.junit.Assert.*;
@SuppressWarnings("ZeroLengthArrayAllocation") @SuppressWarnings("ZeroLengthArrayAllocation")
public class UnpooledTest { public class UnpooledTest {
private static final Queue<ByteBuf> freeLaterQueue = new ArrayDeque<ByteBuf>();
protected ByteBuf freeLater(ByteBuf buf) {
freeLaterQueue.add(buf);
return buf;
}
@After
public void tearDown() {
for (;;) {
ByteBuf buf = freeLaterQueue.poll();
if (buf == null) {
break;
}
if (buf.refCnt() > 0) {
buf.release(buf.refCnt());
}
}
}
@Test @Test
public void testCompositeWrappedBuffer() { public void testCompositeWrappedBuffer() {
ByteBuf header = buffer(12); ByteBuf header = buffer(12);
@ -127,22 +151,22 @@ public class UnpooledTest {
@Test @Test
public void testCompare() { public void testCompare() {
List<ByteBuf> expected = new ArrayList<ByteBuf>(); List<ByteBuf> expected = new ArrayList<ByteBuf>();
expected.add(wrappedBuffer(new byte[] { 1 })); expected.add(wrappedBuffer(new byte[]{1}));
expected.add(wrappedBuffer(new byte[] { 1, 2 })); expected.add(wrappedBuffer(new byte[]{1, 2}));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 })); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10}));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 })); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}));
expected.add(wrappedBuffer(new byte[] { 2 })); expected.add(wrappedBuffer(new byte[]{2}));
expected.add(wrappedBuffer(new byte[] { 2, 3 })); expected.add(wrappedBuffer(new byte[]{2, 3}));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 })); expected.add(wrappedBuffer(new byte[]{2, 3, 4, 5, 6, 7, 8, 9, 10, 11}));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 })); expected.add(wrappedBuffer(new byte[]{2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4 }, 1, 1)); expected.add(wrappedBuffer(new byte[]{2, 3, 4}, 1, 1));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4 }, 2, 2)); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4}, 2, 2));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 }, 1, 10)); expected.add(wrappedBuffer(new byte[]{2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}, 1, 10));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 }, 2, 12)); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}, 2, 12));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4, 5 }, 2, 1)); expected.add(wrappedBuffer(new byte[]{2, 3, 4, 5}, 2, 1));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4, 5 }, 3, 2)); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4, 5}, 3, 2));
expected.add(wrappedBuffer(new byte[] { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 }, 2, 10)); expected.add(wrappedBuffer(new byte[]{2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14}, 2, 10));
expected.add(wrappedBuffer(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 }, 3, 12)); expected.add(wrappedBuffer(new byte[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15}, 3, 12));
for (int i = 0; i < expected.size(); i ++) { for (int i = 0; i < expected.size(); i ++) {
for (int j = 0; j < expected.size(); j ++) { for (int j = 0; j < expected.size(); j ++) {
@ -216,7 +240,7 @@ public class UnpooledTest {
@Test @Test
public void testWrappedBuffer() { public void testWrappedBuffer() {
assertEquals(16, wrappedBuffer(ByteBuffer.allocateDirect(16)).capacity()); assertEquals(16, freeLater(wrappedBuffer(ByteBuffer.allocateDirect(16))).capacity());
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
@ -224,10 +248,10 @@ public class UnpooledTest {
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
wrappedBuffer( freeLater(wrappedBuffer(
new byte[] { 1 }, new byte[] { 1 },
new byte[] { 2 }, new byte[] { 2 },
new byte[] { 3 })); new byte[] { 3 })));
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
@ -237,10 +261,10 @@ public class UnpooledTest {
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
wrappedBuffer( freeLater(wrappedBuffer(
wrappedBuffer(new byte[] { 1 }), wrappedBuffer(new byte[] { 1 }),
wrappedBuffer(new byte[] { 2 }), wrappedBuffer(new byte[] { 2 }),
wrappedBuffer(new byte[] { 3 }))); wrappedBuffer(new byte[] { 3 }))));
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
@ -250,10 +274,10 @@ public class UnpooledTest {
assertEquals( assertEquals(
wrappedBuffer(new byte[] { 1, 2, 3 }), wrappedBuffer(new byte[] { 1, 2, 3 }),
wrappedBuffer( freeLater(wrappedBuffer(
ByteBuffer.wrap(new byte[] { 1 }), ByteBuffer.wrap(new byte[] { 1 }),
ByteBuffer.wrap(new byte[] { 2 }), ByteBuffer.wrap(new byte[] { 2 }),
ByteBuffer.wrap(new byte[] { 3 }))); ByteBuffer.wrap(new byte[] { 3 }))));
} }
@Test @Test