Repository: netty/netty
Issue: Race in PoolArena.allocate
Labels: defect

It appears there might be a race in `io.netty.buffer.PoolArena.allocate`, where `allocationsTiny` is incremented.  While the value is incremented under a lock, it also appears to be mutated elsewhere in the file without a lock.  A race detector suggested the two places where the class accesses this field in an unsynchronized way:

```
  Read of size 8 at 0x7f2f7be16108 by thread T53 (mutexes: write M95278):
    #0 io.netty.buffer.PoolArena.allocate(Lio/netty/buffer/PoolThreadCache;Lio/netty/buffer/PooledByteBuf;I)V (PoolArena.java:198)  
    #1 io.netty.buffer.PoolArena.reallocate(Lio/netty/buffer/PooledByteBuf;IZ)V (PoolArena.java:359)  
    #2 io.netty.buffer.PooledByteBuf.capacity(I)Lio/netty/buffer/ByteBuf; (PooledByteBuf.java:120)  
    #3 io.netty.buffer.AbstractByteBuf.ensureWritable0(I)V (AbstractByteBuf.java:269)  
    #4 io.netty.buffer.AbstractByteBuf.ensureWritable(I)Lio/netty/buffer/ByteBuf; (AbstractByteBuf.java:250)  
```

and

```
    #0 io.netty.buffer.PoolArena.allocate(Lio/netty/buffer/PoolThreadCache;Lio/netty/buffer/PooledByteBuf;I)V (PoolArena.java:198)  
    #1 io.netty.buffer.PoolArena.allocate(Lio/netty/buffer/PoolThreadCache;II)Lio/netty/buffer/PooledByteBuf; (PoolArena.java:133)  
    #2 io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(II)Lio/netty/buffer/ByteBuf; (PooledByteBufAllocator.java:262)  
    #3 io.netty.buffer.AbstractByteBufAllocator.directBuffer(II)Lio/netty/buffer/ByteBuf; (AbstractByteBufAllocator.java:157)  
    #4 io.netty.buffer.AbstractByteBufAllocator.directBuffer(I)Lio/netty/buffer/ByteBuf; (AbstractByteBufAllocator.java:148)  
```

cc: @nmittler & @normanmaurer 


Issue comments:
Will check...

yep that's a possible race, but it doesn't really happen in practice and I think if it does nobody will notice or care :).

It may happen if two threads have the same arena's assigned to them, and those two threads need to concurrently allocate from two different tiny size classes and there must not be any free regions available in the thread caches for both of those two tiny size classes (same is true for `allocationsSmall`).

However, by default Netty settings, each thread has its own arena (cores \* 2) and almost all tiny allocations should be served by the cache. If it happens it shouldn't matter, cause `allocationsTiny` and `allocationsSmall` are just counter variables for stats, that aren't really in use.

Just saying ... still a bug :smile: 

@buchgr I agree... I'm not sure if it really worth to care here and take the performance hit. That said maybe we should and just allow to disable recording metrics. WDYT ?

@normanmaurer up to you :-). I think correctness goes over performance and options for disabling/enabling is just adding unnecessary complexity. That being said, I think if the thread caches work correctly (>99.9% hit rate), there will not be a performance impact.

You can also leave this open for me to have a look at in ~4 weeks. I am planning to add some stat reporting to the allocator, to better understand what's happening in there for grpc.

@buchgr let me maybe just fix it for now and we can revisit later. WDYT ?

@normanmaurer sgtm

@buchgr fwiw the race detector found this when running our unit tests, which I believe are not atypical users of netty.  The race detector is similar to valgrind, which reports suspicious memory accesses.  To this end, I don't believe the race is hypothetical.

@carl-mastrangelo do you know which unit test it was? If my understanding of the code is correct, a necessary condition for the race to happen would be that this test uses more than `2 * ncores` many event loops? You are right in saying that it's not hypothetical, it can happen, but if it happens nothing happens so to say :). it's just an unused stat counter. I am all for fixing it.

@carl-mastrangelo that would actually be really helpful. If it happens with lte `2 * ncores` threads, then this might point at the logic assigning arenas to threads having a bug, which would be important to fix.

@buchgr I found it in this test: https://github.com/grpc/grpc-java/blob/master/interop-testing/src/test/java/io/grpc/testing/integration/Http2NettyLocalChannelTest.java  but It happens for most of the other tests in the same directory.

If it is infact a harmless race, then its probably okay.  I don't think I can configure my race detector to ignore particular races, so fixing this would help de-noise my test runs.

@carl-mastrangelo thanks! I ll take a look. I assume the race detector is google internal?

Yeah, sadly.  Otherwise I would be more forthcoming with details.  

Any chance to opensource it ? **hint**

> Am 04.02.2016 um 18:16 schrieb Carl Mastrangelo notifications@github.com:
> 
> Yeah, sadly. Otherwise I would be more forthcoming with details.
> 
> —
> Reply to this email directly or view it on GitHub.

Fixed by https://github.com/netty/netty/pull/4974


Pull request review comments:

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/PoolArena.java b/buffer/src/main/java/io/netty/buffer/PoolArena.java
index 4cb6af2b6a7..33445241b7b 100644
--- a/buffer/src/main/java/io/netty/buffer/PoolArena.java
+++ b/buffer/src/main/java/io/netty/buffer/PoolArena.java
@@ -57,8 +57,8 @@ enum SizeClass {
     private final List<PoolChunkListMetric> chunkListMetrics;
 
     // Metrics for allocations and deallocations
-    private long allocationsTiny;
-    private long allocationsSmall;
+    private LongCounter allocationsTiny = PlatformDependent.newLongCounter();
+    private LongCounter allocationsSmall = PlatformDependent.newLongCounter();
     private long allocationsNormal;
     // We need to use the LongCounter here as this is not guarded via synchronized block.
     private final LongCounter allocationsHuge = PlatformDependent.newLongCounter();
@@ -195,9 +195,9 @@ private void allocate(PoolThreadCache cache, PooledByteBuf<T> buf, final int req
                     s.chunk.initBufWithSubpage(buf, handle, reqCapacity);
 
                     if (tiny) {
-                        ++allocationsTiny;
+                        allocationsTiny.increment();
                     } else {
-                        ++allocationsSmall;
+                        allocationsSmall.increment();
                     }
                     return;
                 }
@@ -432,17 +432,17 @@ private static List<PoolSubpageMetric> subPageMetricList(PoolSubpage<?>[] pages)
 
     @Override
     public long numAllocations() {
-        return allocationsTiny + allocationsSmall + allocationsNormal + allocationsHuge.value();
+        return allocationsTiny.value() + allocationsSmall.value() + allocationsNormal + allocationsHuge.value();
     }
 
     @Override
     public long numTinyAllocations() {
-        return allocationsTiny;
+        return allocationsTiny.value();
     }
 
     @Override
     public long numSmallAllocations() {
-        return allocationsSmall;
+        return allocationsSmall.value();
     }
 
     @Override


Test patch: