Fix ByteBuffer Thread-Safety in WebSocket Binary Message Handling

[xeromank]

Summary

This PR addresses a thread-safety issue when sending binary WebSocket messages to multiple sessions using a shared ByteBuffer. The fix ensures that each session receives an independent buffer view, preventing concurrent modification issues.

Background

Following the previous PR discussion, this implementation takes a different approach by handling ByteBuffer safety at the session level rather than in ConcurrentWebSocketSessionDecorator.

The Problem

When sending a BinaryMessage with the same ByteBuffer to multiple WebSocket sessions, the buffer's position is shared across all send operations. This causes:

• First session receives the complete data
• Subsequent sessions receive empty or partial data
• Potential race conditions in concurrent environments

Why This Matters

1. Developer Awareness: The internal structure of BinaryMessage and ByteBuffer position sharing is not immediately obvious to developers
2. Virtual Threads: With the adoption of virtual threads, this issue becomes inevitable as more concurrent operations share the same resources
3. Framework Responsibility: While sharing ByteBuffers across threads is technically incorrect usage, the framework should protect against common mistakes

The Solution

Use ByteBuffer.asReadOnlyBuffer() in both StandardWebSocketSession and JettyWebSocketSession to create independent buffer views for each send operation.

Changes

• StandardWebSocketSession.sendBinaryMessage(): Use message.getPayload().asReadOnlyBuffer()
• JettyWebSocketSession.sendBinaryMessage(): Use message.getPayload().asReadOnlyBuffer()
• Added comprehensive tests verifying buffer independence

Alignment with WebFlux

Spring WebFlux already implements this pattern:

WebFlux DataBuffer Implementations:

• NettyDataBuffer.readableByteBuffers() - Creates new ByteBuffer arrays
• JettyDataBuffer.readableByteBuffers() - Wraps with iterator for safe access

WebFlux WebSocket Sessions:

• NettyWebSocketSessionSupport - Handles ByteBuffer safely
• JettyWebSocketSession - Implements safe buffer handling

There's no reason why Spring MVC WebSocket should handle this differently than WebFlux. Both face the same thread-safety challenges, and consistency across the framework is important.

Testing

Added tests to verify:

1. Multiple sessions receive complete data when sharing a ByteBuffer
2. Original buffer position/limit remain unchanged after sends
3. Each session gets an independent buffer view

Performance Impact

• asReadOnlyBuffer() creates a shallow copy (shares the underlying data)
• Minimal overhead - only duplicates buffer metadata (position, limit, mark)
• No additional memory allocation for the actual data

Conclusion

This change brings Spring MVC WebSocket in line with WebFlux's approach to ByteBuffer handling, providing better thread-safety by default while maintaining backwards compatibility and performance.

[bclozel]

Making the ByteBuffer read-only will:

• make their position/mark/limit to be independent from other instances
• will prevent the buffer from being written to (which Framework is not doing anyway)

But this will not prevent the original buffer from being written to, which will cause the data written to the outgoing websocket session to be garbled. In my opinion, this doesn't improve the situation with your use case about using the same bytebuffer in a concurrent fashion over several sessions. If anything, the asReadOnlyBuffer() call should be made in your application code because you chose to use the same instance for several sessions.

[xeromank]

@bclozel

Thank you for the feedback. Let me clarify the actual concurrency issue:

The Real Problem: Position Race Conditions in Concurrent Access

You're right that this isn't about data corruption. The issue is race conditions on ByteBuffer's position when multiple threads access it concurrently.

What Actually Happens in Concurrent Execution:

ByteBuffer buffer = ByteBuffer.wrap(data); // position = 0

// Multiple threads executing concurrently:
Thread1: session1.sendMessage(new BinaryMessage(buffer));
Thread2: session2.sendMessage(new BinaryMessage(buffer));
Thread3: session3.sendMessage(new BinaryMessage(buffer));

// Inside sendBinary, each thread does:
// 1. Read from current position
// 2. Advance position while reading
// 3. Position returns to 0 after fully read

// Race condition example:
// Thread1: reading byte 50 of 100 (position = 50)
// Thread2: starts reading from position 50 (gets partial data!)
// Thread1: finishes reading (position back to 0)
// Thread3: starts reading from position 0 (gets full data)

Result: Some sessions receive partial data, some receive full data, depending on timing.

This is Not About Sequential Processing

The problem isn't that "position is at the end after first send." It's that during concurrent sends, threads interfere with each other's reading position, causing:

• Partial message delivery
• Garbled data (mixing different parts of the buffer)
• Unpredictable behavior based on thread scheduling

Why Position Independence Matters

With asReadOnlyBuffer(), each thread gets its own position/limit/mark:
// Each thread gets independent position tracking
Thread1: buffer.asReadOnlyBuffer() → position changes don't affect others
Thread2: buffer.asReadOnlyBuffer() → own position, reads full data
Thread3: buffer.asReadOnlyBuffer() → own position, reads full data

WebFlux Already Solved This

WebFlux recognized this exact issue:

• https://github.com/spring-projects/spring-framework/blob/main/spring-core/src/main/java/org/springframework/core/io/buffer/NettyDataBuffer.java#L320-L323 - Creates independent ByteBuffer views
• They understood that concurrent ByteBuffer access is common in reactive/async code

High Concurrency is the New Normal

With virtual threads and reactive patterns:

  // My broadcast pattern
  ByteBuffer notification = ByteBuffer.wrap(data);
  sessions.parallelStream().forEach(session ->
      session.sendMessage(new BinaryMessage(notification))
  );

I don't think this is the correct pattern.
The framework should handle this.

Why Framework Should Handle This

1. ByteBuffer's shared position is a low-level detail - Most developers don't expect reading to be stateful
2. The intent is clear - "Send same message to multiple sessions"
3. It's a one-line fix in the framework vs. every developer learning this pitfall
4. WebFlux already established this as a framework responsibility

The question isn't "should applications share ByteBuffers?" but rather "when they do (and they will), should the framework make it safe?"

[xeromank]

@bclozel
I'd be grateful for your feedback on this approach.

[bclozel]

@xeromank I'm still not convinced. The example you're pointing at for WebFlux is the other way around: those are buffers provided by the Framework to the application. If it was the case, then all WebFlux Encoder would copy the DataBuffer as read-only before writing them for serialization.

[xeromank]

@bclozel

I think the reason webflux provide such buffers is ultimately because ByteBuffer exists internally, right? Actually, I don’t understand why WebFlux was designed that way. But at least, to use MVC WebSocket in an application, you have to use WebSocketMessage, and this model is provided by the framework. While it’s true that developers provide ByteBuffer to use BinaryMessage, wouldn’t it be natural to assume that internally, elements like position would be used in a thread-safe manner?
I’d like your opinion on this.
I’m in Seoul and it’s 1 AM now, so I’ll check your response tomorrow. Thank you so much.

[bclozel]

Let's agree to disagree, then.
I don't think this is a Spring Framework problem, as it's a typical Java pattern: if an application provides the same ByteBuffer instance to different processing units, this will happen. Same for collections, and anything mutable. The limit/mark/position is part of the state of the ByteBuffer instance.

[xeromank]

@bclozel

Thanks for the quick feedback.
But I think the important thing is that this isn’t just a simple mutable object - it’s a stream object that changes position state during serialization. Of course, when using Java, we shouldn’t overlook this point, but I believe many developers don’t realize that ByteBuffer is a stream object. From that perspective, wouldn’t it be great if the framework took responsibility for this? Because there’s no alternative model to use either…

[xeromank]

And to add to that, I think someone will definitely have the same experience as me. And they’ll look at Spring and Tomcat code and think: ‘Why doesn’t it guarantee thread safety when using BinaryMessage?’

[xeromank]

And does that mean we should use it like this?

fun sendMessageToFiltered(
    payload: ByteArray,
    filterKey: String,
    filterValue: Any = true
) {
    sessions.keys.filter { session ->
        session.get(filterKey) == filterValue
    }.forEach { session ->
        sessions[session]?.sendMessage(BinaryMessage(payload))
    }
}

I might be misunderstanding, but I think the following code represents a more typical usage pattern:

fun sendMessageToFiltered(
    message: WebSocketMessage<*>,
    filterKey: String,
    filterValue: Any = true
) {
    sessions.keys.filter { session ->
        session.get(filterKey) == filterValue
    }.forEach { session ->
        sessions[session]?.sendMessage(message)
    }
}

Because when you look at ConcurrentWebSocketSessionDecorator, the signature of sendMessage is as follows, so I think it’s reasonable to interpret it this way. Otherwise, wouldn’t it be clearer if functions were overloaded for each specific message class?

public void sendMessage(WebSocketMessage<?> message) throws IOException {

That’s why I think the framework should guarantee thread safety at the framework level to prevent ByteBuffer position issues.

[xeromank]

@bclozel
I hope I haven't been forgotten. I'm still waiting for a response to the PR comments.

[bclozel]

Let me have another look, I'll update you here.

[xeromank]

@bclozel
Thank you! I appreciate you taking the time to reconsider this. Looking forward to your feedback.

[bclozel]

I had another look at the 4 PRs, namely: #35517, #35518, #35519 and #35520.
Each one brings a lot of information and various changes. You've stated that english is not your native language (and this is never a problem in this issue tracker). But here, using an LLM to write the PR description is definitely not helping. It's drowning the important parts under loads of text and makes triage harder than it should.

Now, focusing on the use case you have brought up, that can be summarized like this:

When sending a binary message to multiple websocket sessions, there is a concurrency problem with the following pattern:

  // My broadcast pattern
 ByteBuffer notification = ByteBuffer.wrap(data);
 sessions.parallelStream().forEach(session ->
     session.sendMessage(new BinaryMessage(notification))
 );

if the message is read from multiple sessions at the same time, the data we send is not correct

I agree that sending the same binary message is a bit tricky here, because you need to do something like this:

// My broadcast pattern
ByteBuffer notification = ByteBuffer.wrap(data);
sessions.parallelStream().forEach(session ->
   session.sendMessage(new BinaryMessage(notification.asReadOnlyBuffer()))
);

That's assuming that the buffer won't be written to while messages are being sent. This is the very nature of ByteBuffer: they pack both the data and indices for reading and writing. They are not thread safe. Popular libraries create pools of buffers to recycle them after use, and they ensure that they are not used concurrently.

Now I have considered a change to make that slightly easier, making the buffer read-only when we instantiate the BinaryMessage.

A general pattern in Spring messaging to create immutable org.springframework.messaging.Message instances and copy/modify them with a builder. But the payload itself can be stateful and this is a problem that needs to be dealt with by the application.

Here, the websocket message does not extend org.springframework.messaging.Message but the same principles apply.
A BinaryMessage can be backed by a byte[] or a ByteBuffer. In both cases, the bytes are still mutable and can be mutated externally. For example:

byte[] spring = "spring".getBytes(StandardCharsets.UTF_8);
BinaryMessage msg = new BinaryMessage(spring);
// mutating the spring byte array changes the message payload

ByteBuffer buffer = ByteBuffer.wrap("spring".getBytes(StandardCharsets.UTF_8));
BinaryMessage bufferMessage = new BinaryMessage(buffer);
// mutating the buffer changes the message payload

Making the payload immutable without the developer's knowledge is likely to create regressions and this will be inconsistent compared to using raw byte arrays.

In summary, I think we have considered many angles but we cannot change the behavior here. When dealing with BinaryMessage, developers should ensure that they understand the basics about ByteBuffer when they use them.

[xeromank]

@bclozel

Thank you so much for the detailed and thoughtful comment. I really appreciate the time you took to thoroughly review this.

I completely agree that developers should understand the basics of ByteBuffer and byte[]. However, I don't think any developer would intentionally modify them after passing that information to BinaryMessage. Writing code with such structure would be extremely rare.

My perspective isn't about modifying ByteBuffer or byte[] - it's about the position issues that can occur in multi-threaded environments when each server (e.g., Tomcat) extracts ByteBuffers from BinaryMessage for transmission, and multiple threads extract the same ByteBuffer concurrently.

To resolve this, applications would need to create as many BinaryMessage instances as there are threads. But I think it's more efficient to internally create ByteBuffers that have different state values while pointing to the same byte array data, rather than requiring applications to handle this complexity.

The core issue is that when the framework extracts the ByteBuffer for actual transmission, multiple threads sharing the same ByteBuffer instance will interfere with each other's position/limit/mark state, not that the underlying data gets modified.

What are your thoughts on this perspective? This is the core of my thinking.

[bclozel]

You are making a valid point and I agree. As stated in my previous comment, I considered changing the existing implementation but decided against it because this would break existing behavior and would be inconsistent with all other message payloads. While you consider this a rare case, applying this change would make it impossible to access the payload in a mutable fashion, with no workaround possible. Other developers would say that reusing the same buffer in a concurrent manner is niche. It is a matter of perspective.

If we get more similar feedback from the community we can reconsider but unfortunately in the meantime we will have to leave it at that.

[xeromank]

@bclozel
Thank you for your response!
Ah, you're right - if there are any existing use cases, we wouldn't be able to guarantee backward compatibility at all. I hadn't considered that perspective. From that standpoint, I understand your decision. But then, would it make sense for me to try creating a different Decorator by extending the existing one?

[bclozel]

You can extend it for your own purposes in your application. I don't think we're going to evolve the API for this reason.
The BinaryMessage contract has been around since Spring Framework 4.0 (2013) and to my knowledge this is the first time we receive this kind of feedback.

[xeromank]

@bclozel
Thank you for the feedback.

I probably wouldn't have encountered this issue if I hadn't been using virtual threads either.

Have a great day!