I wrote a WebSocket server in Java. This is the method that the server uses to send WebSocket packets to its clients:

private void sendFrame(boolean fin, boolean rsv1, boolean rsv2, boolean rsv3, WebSocketOpcode opcode, byte[] payloadData) throws IOException {
  if (connection.isClosed() || webSocketConnectionClosing != null) return;
  byte[] header = new byte[2];
  if (fin) header[0] |= 1 << 7;
  if (rsv1) header[0] |= 1 << 6;
  if (rsv2) header[0] |= 1 << 5;
  if (rsv3) header[0] |= 1 << 4;
  header[0] |= opcode.get() & 0b1111;
  header[1] |= payloadData.length < 126 ? payloadData.length : (payloadData.length <= 65535 ? 126 : 127);
  out.write(header);
  if (payloadData.length > 125) {
    if (payloadData.length <= 65535) {
      out.writeShort(payloadData.length);
    } else {
      out.writeLong(payloadData.length);
    }
  }
  out.write(payloadData);
  out.flush();
}

And this is how I declare the output stream after a client connects:

out = new DataOutputStream(new BufferedOutputStream(connection.getOutputStream()));

And I have some questions regarding this:

Is the above code thread-safe? What I mean by that is, can multiple threads call sendFrame() at the same time without the risk of packets data interleaving? It looks like this code is wrong, but I haven't encountered any interleaving yet.

If it isn't thread-safe, then how would I make it thread-safe in this form without the use of queues? (I want the sendFrame() method to be blocking until the data is actually sent)

If I wouldn't wrap the OutputStream in BufferedOutputStream, but only in DataOutputStream instead, would this make the .write() method atomic? Would it be thread-safe to pack the entire packet data into a single byte array and then call .write() once with that array?

CodePudding user response：

Is the above code thread-safe? What I mean by that is, can multiple threads call sendFrame() at the same time without the risk of packets data interleaving?

It is not thread-safe.

It looks like this code is wrong, but I haven't encountered any interleaving yet.

The time window in which the interleaving could occur is very small. Probably less than a microsecond. That means the probability of it occurring is small. But not zero.

If it isn't thread-safe, then how would I make it thread-safe in this form without the use of queues? (I want the sendFrame() method to be blocking until the data is actually sent)

It depends on how the sendFrame method fits in with the rest of your code.

The approach I would used would be to ensure that all calls to sendFrame for a specific output stream and being made on the same target object. Then I would use synchronized to lock on the target object or a private log belonging to the target object.

An alternative would be to use synchronized and lock on out. However there is a risk that something else is doing that already, and sendFrame calls would be blocked unnecessarily.

If I wouldn't wrap the OutputStream in BufferedOutputStream, but only in DataOutputStream instead, would this make the .write() method atomic?

(That's beside the point. You have 3 write calls to contend with. However ....)

Would it be thread-safe to pack the entire packet data into a single byte array and then call .write() once with that array?

None of those classes are documented¹ as thread-safe, or as guaranteeing that write operations are atomic. However, in OpenJDK Java 11 (at least), the relevant write methods are implemented as synchronized in BufferedOutputStream and DataOutputStream.

^{1 - If the javadocs don't specify thread-safety, etc characteristics, then those characteristics could vary depending on the Java version, etc.}