The WebSocketWriter is single-threaded. (square#3571)

We were synchronizing to permit multiple writer threads. But that was a carry-over
from the previous design where we supported multiple writer threads. With the
current implementation only one thread writes at a time. This synchronization
was not necessary.

Author: swankjesse

okhttp/src/main/java/okhttp3/internal/ws/WebSocketReader.java

@@ -46,8 +46,8 @@
 
 /**
  * An <a href="http://tools.ietf.org/html/rfc6455">RFC 6455</a>-compatible WebSocket frame reader.
- * <p>
- * This class is not thread safe.
+ *
+ * <p>This class is not thread safe.
  */
 final class WebSocketReader {
   public interface FrameCallback {

okhttp/src/main/java/okhttp3/internal/ws/WebSocketWriter.java

@@ -39,10 +39,7 @@
 /**
  * An <a href="http://tools.ietf.org/html/rfc6455">RFC 6455</a>-compatible WebSocket frame writer.
  *
- * <p>This class is partially thread safe. Only a single "main" thread should be sending messages
- * via calls to {@link #newMessageSink}, {@link #writePing}, or {@link #writeClose}. Other threads
- * may call {@link #writePing}, {@link #writePong}, or {@link #writeClose} which will interleave on
- * the wire with frames from the "main" sending thread.
+ * <p>This class is not thread safe.
  */
 final class WebSocketWriter {
   final boolean isClient;
@@ -75,16 +72,12 @@ final class WebSocketWriter {
 
   /** Send a ping with the supplied {@code payload}. */
   void writePing(ByteString payload) throws IOException {
-    synchronized (this) {
-      writeControlFrameSynchronized(OPCODE_CONTROL_PING, payload);
-    }
+    writeControlFrame(OPCODE_CONTROL_PING, payload);
   }
 
   /** Send a pong with the supplied {@code payload}. */
   void writePong(ByteString payload) throws IOException {
-    synchronized (this) {
-      writeControlFrameSynchronized(OPCODE_CONTROL_PONG, payload);
-    }
+    writeControlFrame(OPCODE_CONTROL_PONG, payload);
   }
 
   /**
@@ -108,18 +101,14 @@ void writeClose(int code, ByteString reason) throws IOException {
       payload = buffer.readByteString();
     }
 
-    synchronized (this) {
-      try {
-        writeControlFrameSynchronized(OPCODE_CONTROL_CLOSE, payload);
-      } finally {
-        writerClosed = true;
-      }
+    try {
+      writeControlFrame(OPCODE_CONTROL_CLOSE, payload);
+    } finally {
+      writerClosed = true;
     }
   }
 
-  private void writeControlFrameSynchronized(int opcode, ByteString payload) throws IOException {
-    assert Thread.holdsLock(this);
-
+  private void writeControlFrame(int opcode, ByteString payload) throws IOException {
     if (writerClosed) throw new IOException("closed");
 
     int length = payload.size();
@@ -169,10 +158,8 @@ Sink newMessageSink(int formatOpcode, long contentLength) {
     return frameSink;
   }
 
-  void writeMessageFrameSynchronized(int formatOpcode, long byteCount, boolean isFirstFrame,
+  void writeMessageFrame(int formatOpcode, long byteCount, boolean isFirstFrame,
       boolean isFinal) throws IOException {
-    assert Thread.holdsLock(this);
-
     if (writerClosed) throw new IOException("closed");
 
     int b0 = isFirstFrame ? formatOpcode : OPCODE_CONTINUATION;
@@ -235,19 +222,15 @@ final class FrameSink implements Sink {
 
       long emitCount = buffer.completeSegmentByteCount();
       if (emitCount > 0 && !deferWrite) {
-        synchronized (WebSocketWriter.this) {
-          writeMessageFrameSynchronized(formatOpcode, emitCount, isFirstFrame, false /* final */);
-        }
+        writeMessageFrame(formatOpcode, emitCount, isFirstFrame, false /* final */);
         isFirstFrame = false;
       }
     }
 
     @Override public void flush() throws IOException {
       if (closed) throw new IOException("closed");
 
-      synchronized (WebSocketWriter.this) {
-        writeMessageFrameSynchronized(formatOpcode, buffer.size(), isFirstFrame, false /* final */);
-      }
+      writeMessageFrame(formatOpcode, buffer.size(), isFirstFrame, false /* final */);
       isFirstFrame = false;
     }
 
@@ -259,9 +242,7 @@ final class FrameSink implements Sink {
     @Override public void close() throws IOException {
       if (closed) throw new IOException("closed");
 
-      synchronized (WebSocketWriter.this) {
-        writeMessageFrameSynchronized(formatOpcode, buffer.size(), isFirstFrame, true /* final */);
-      }
+      writeMessageFrame(formatOpcode, buffer.size(), isFirstFrame, true /* final */);
       closed = true;
       activeWriter = false;
     }