Repository: netty/netty
Issue: Inconsistent synchronization of some variables in SpdySessionHandler
Labels: defect

1. Field `io.netty.handler.codec.spdy.SpdySessionHandler.lastGoodStreamId`
   Synchronized 50% of the time:
   Unsynchronized access at SpdySessionHandler.java:[line 148]
   Unsynchronized access at SpdySessionHandler.java:[line 243]
   Synchronized access at SpdySessionHandler.java:[line 736]
   Synchronized access at SpdySessionHandler.java:[line 851]
2. Field `io.netty.handler.codec.spdy.SpdySessionHandler.receivedGoAwayFrame`
   Synchronized 50% of the time
   Unsynchronized access at SpdySessionHandler.java:[line 361]
   Synchronized access at SpdySessionHandler.java:[line 723]
3. Field `io.netty.handler.codec.spdy.SpdySessionHandler.localConcurrentStreams`
   Synchronized 50% of the time
   Unsynchronized access at SpdySessionHandler.java:[line 598]
   Synchronized access at SpdySessionHandler.java:[line 728]
4. Field `io.netty.handler.codec.spdy.SpdySessionHandler.sentGoAwayFrame`
   Synchronized 75% of the time
   Unsynchronized access at SpdySessionHandler.java:[line 150]
   Synchronized access at SpdySessionHandler.java:[line 723]
   Synchronized access at SpdySessionHandler.java:[line 849]
   Synchronized access at SpdySessionHandler.java:[line 850]
5. Field `io.netty.handler.codec.spdy.SpdySessionHandler.remoteConcurrentStreams`
   Synchronized 50% of the time
   Unsynchronized access at SpdySessionHandler.java:[line 318]
   Synchronized access at SpdySessionHandler.java:[line 728]

Message from FindBugs:
The fields of this class appear to be accessed inconsistently with respect to synchronization.  This bug report indicates that the bug pattern detector judged that
The class contains a mix of locked and unlocked accesses,
The class is not annotated as javax.annotation.concurrent.NotThreadSafe,
At least one locked access was performed by one of the class's own methods, and
The number of unsynchronized field accesses (reads and writes) was no more than one third of all accesses, with writes being weighed twice as high as reads
A typical bug matching this bug pattern is forgetting to synchronize one of the methods in a class that is intended to be thread-safe.
You can select the nodes labeled "Unsynchronized access" to show the code locations where the detector believed that a field was accessed without synchronization.
Note that there are various sources of inaccuracy in this detector; for example, the detector cannot statically detect all situations in which a lock is held.  Also, even when the detector is accurate in distinguishing locked vs. unlocked accesses, the code in question may still be correct.

@trustin @normanmaurer @fredericBregier @jpinner 
Please, check it out.


Issue comments:
@jpinner I may miss something but I think in netty 4+ we don't need the synchronized stuff at all because of the new thread-model. Can you confirm ?

@jpinner ping :)

@jpinner could you please check so we can fix this... Thanks !

Let me fix this


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
index 6e540256321..338456be5a7 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/spdy/SpdySessionHandler.java
@@ -700,21 +700,21 @@ private boolean isRemoteInitiatedId(int id) {
     }
 
     // need to synchronize to prevent new streams from being created while updating active streams
-    private synchronized void updateInitialSendWindowSize(int newInitialWindowSize) {
+    private void updateInitialSendWindowSize(int newInitialWindowSize) {
         int deltaWindowSize = newInitialWindowSize - initialSendWindowSize;
         initialSendWindowSize = newInitialWindowSize;
         spdySession.updateAllSendWindowSizes(deltaWindowSize);
     }
 
     // need to synchronize to prevent new streams from being created while updating active streams
-    private synchronized void updateInitialReceiveWindowSize(int newInitialWindowSize) {
+    private void updateInitialReceiveWindowSize(int newInitialWindowSize) {
         int deltaWindowSize = newInitialWindowSize - initialReceiveWindowSize;
         initialReceiveWindowSize = newInitialWindowSize;
         spdySession.updateAllReceiveWindowSizes(deltaWindowSize);
     }
 
     // need to synchronize accesses to sentGoAwayFrame, lastGoodStreamId, and initial window sizes
-    private synchronized boolean acceptStream(
+    private boolean acceptStream(
             int streamId, byte priority, boolean remoteSideClosed, boolean localSideClosed) {
         // Cannot initiate any new streams after receiving or sending GOAWAY
         if (receivedGoAwayFrame || sentGoAwayFrame) {
@@ -833,7 +833,7 @@ private void sendGoAwayFrame(ChannelHandlerContext ctx, ChannelPromise future) {
         // FIXME: Close the connection forcibly after timeout.
     }
 
-    private synchronized ChannelFuture sendGoAwayFrame(
+    private ChannelFuture sendGoAwayFrame(
             ChannelHandlerContext ctx, SpdySessionStatus status) {
         if (!sentGoAwayFrame) {
             sentGoAwayFrame = true;


Test patch: