Skip to content

Conversation

xeromank
Copy link

@xeromank xeromank commented Sep 22, 2025

For reference, I'm not good at English, so I wrote this message using a translator. Please understand if my expressions are a bit awkward or if there are any issues.

📝 Summary

With Spring's growing adoption of virtual threads (Project Loom), concurrency levels in applications have increased dramatically. This exposed a critical thread-safety issue in ConcurrentWebSocketSessionDecorator when handling binary messages.

The Problem: When multiple threads send the same BinaryMessage to different WebSocket sessions, they share a single ByteBuffer whose position gets corrupted during concurrent reads, resulting in data corruption or empty messages.

Why This Is a Trap: Developers using binary messages might not be aware that ByteBuffer's position is mutable state. It's natural to assume that sending the same message to multiple sessions is safe - after all, we're just "reading" the data. This hidden implementation detail makes this bug easy to accidentally introduce and particularly challenging to diagnose.

I was also unaware of this implementation detail and only discovered it after spending two days debugging in production when concurrency levels increased dramatically. I hope this fix saves others from the same frustrating experience.

🔄 Changes

  • Added prepareMessage() method to handle message preparation
  • Use ByteBuffer.duplicate() for BinaryMessage to ensure thread-safe position tracking
  • Each thread now gets its own independent buffer position while sharing underlying data
private WebSocketMessage<?> prepareMessage(WebSocketMessage<?> message) {
    if (message instanceof BinaryMessage) {
        ByteBuffer payload = ((BinaryMessage) message).getPayload();
        return new BinaryMessage(payload.duplicate(), message.isLast());
    }
    return message;
}

✅ Testing

Added test concurrentBinaryMessageSharingAcrossSessions() that reproduces the issue and validates the fix.

  • Without fix: Test fails with corrupted buffers
  • With fix: All messages delivered correctly

📊 Impact

  • Performance: Minimal - only creates buffer wrapper, no data copying
  • Compatibility: Fully backwards compatible
  • Virtual Threads: Essential for applications using virtual threads with high concurrency

🏗️ Why Fix in ConcurrentWebSocketSessionDecorator?

You might wonder why this fix was implemented in ConcurrentWebSocketSessionDecorator rather than the base AbstractWebSocketSession. The reasoning is architectural:

  • Blocking I/O Handles This Differently: In traditional blocking I/O implementations (like Tomcat's WsRemoteEndpointImplBase), the sendMessageBlockInternal method calls payload.clear() after sending, resetting the buffer position. This automatic reset prevents position conflicts when reusing the same BinaryMessage in sequential operations.

  • Concurrent Decorator's Purpose: ConcurrentWebSocketSessionDecorator was specifically designed to handle concurrent access scenarios. When multiple threads access the same message concurrently, there's no automatic buffer reset between operations. It's the appropriate layer to address these concurrency-related ByteBuffer position conflicts.

  • Separation of Concerns: Keeping the fix in the decorator maintains clean separation - the base session handles core functionality while the decorator handles thread-safety concerns. The base implementation can rely on the underlying WebSocket container's behavior (like Tomcat's buffer clearing), while the decorator ensures safety in concurrent scenarios.

This design choice ensures the fix is applied exactly where it's needed - in concurrent scenarios - without adding overhead to single-threaded use cases where the container already handles buffer state management.

🎯 Why This Matters Now

As Spring applications increasingly adopt virtual threads:

  • Concurrency levels can reach thousands of concurrent operations
  • Traditional thread-safety assumptions may no longer hold
  • Subtle race conditions become more prevalent

This fix ensures WebSocket handling remains robust in the virtual thread era.


Note: This issue only manifests under high concurrency, particularly with virtual threads, making it challenging to detect in traditional testing scenarios.

- Test concurrent sending of shared BinaryMessage across multiple sessions
- Demonstrates ByteBuffer position corruption in multi-threaded environment
- Validates the need for ByteBuffer.duplicate() when handling binary messages

Signed-off-by: xeroman.k <xeroman.k@gmail.com>
- Duplicate ByteBuffer before sending to avoid position conflicts
- Ensure thread-safe handling of binary messages across multiple sessions

Signed-off-by: xeroman.k <xeroman.k@gmail.com>
@bclozel
Copy link
Member

bclozel commented Sep 22, 2025

Thank you for your contribution to the Spring Framework! We appreciate the time and effort you’ve put into this PR, and we value the community’s engagement in improving the project.

After reviewing the proposed changes, we’ve decided to close this pull request for the following reason:

The current implementation of ConcurrentWebSocketSessionDecorator is designed to allow multiple threads to interact with a WebSocket session concurrently. However, the proposed change assumes that multiple threads using a shared ByteBuffer instance is a valid use case.

ByteBuffer is not thread-safe by design. If multiple threads attempt to read from or write to the same ByteBuffer instance simultaneously, it can lead to race conditions, data corruption, or unexpected behavior. This is especially problematic in high-concurrency scenarios, where WebSocket sessions are often accessed by multiple threads.

The ConcurrentWebSocketSessionDecorator is explicitly intended to enable thread-safe operations on a WebSocket session. Sharing a single ByteBuffer across threads undermines this intent, as it effectively couples the threads’ operations in a way that violates the decorator’s purpose. Each thread should ideally work with its own buffer or coordinate access explicitly if sharing is required.

While reusing a ByteBuffer might reduce object allocation overhead, the potential for subtle bugs and thread-safety violations outweighs the performance benefits. In the context of WebSocket communication, correctness and predictability are paramount.

If the goal is to optimize buffer usage, we recommend exploring the following alternatives in your application:

  1. Use thread-local storage or a pool of ByteBuffer instances to avoid contention while still reducing allocation overhead.
  2. If sharing a buffer is unavoidable, introduce explicit synchronization (e.g., synchronized blocks or locks) to ensure thread safety. However, this should be done cautiously, as it may introduce performance bottlenecks.

Thanks

@bclozel bclozel closed this Sep 22, 2025
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants