[UNDERTOW-2532] Fix NPE when transmitting text or binary message to websocket session at the same time #1708
Conversation
raccoonback
force-pushed
the
UNDERTOW-2532
branch
3 times, most recently
from
c2d0ef2 to
5811376
Compare
baranowb
added
bug fix
… binary message to websocket session
raccoonback
force-pushed
the
UNDERTOW-2532
branch
from
5811376 to
3a105d1
Compare
| if (isLast) { | ||
| textFrameSender = null; | ||
| try { | ||
| Channels.writeBlocking(textFrameSender, WebSocketUtils.fromUtf8String(partialMessage)); |
There was a problem hiding this comment.
Whole section starting with Channels.writeBlocking() and ending with Channes.flushBlocking() must be executed outside of intrinsic lock section otherwise it will introduce deadlock possibility.
There was a problem hiding this comment.
@ropalka That's a good comment! 👍
I will change the section so that there is no impact on the intrinsic lock.
There was a problem hiding this comment.
@ropalka
As you mentioned, the section "Channels.writeBlocking() ~ Channels.flushBlocking()" was excluded from the intrinsic lock range.
Please review again.
| if (binaryFrameSender == null) { | ||
| binaryFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.BINARY); | ||
| } | ||
| try { |
There was a problem hiding this comment.
Same here, see comment above.
…smitting partial messages through the websocket channel
raccoonback
force-pushed
the
UNDERTOW-2532
branch
from
5f77692 to
ccc3ae5
Compare
| binaryFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.BINARY); | ||
| } | ||
|
|
||
| StreamSinkFrameChannel sender = getBinaryFrameSender(); |
There was a problem hiding this comment.
Im going to talk about this with @ropalka . But it seems its technically possible for competing send to undercut one another, though I might be missing how this piece is being used.
There was a problem hiding this comment.
@baranowb
I would like to share my understanding and seek your feedback.
For example, in the case of sending a Text message, I assume that a single message is divided into multiple frames for transmission. In this process, the TextFrameSender class variable is used to call sendText(partialMessage, isLast) multiple times, sending partial messages through the same channel. (During this process, other send() methods are not allowed to send messages.)
However, if partial frames are being sent from multiple threads, I believe there is a possibility that one thread might set textFrameSender to null first, and then another thread could call Channels.flushBlocking(StreamSinkFrameChannel), leading to a NullPointerException (NPE).
To address this, I have modified the code to ensure synchronization when accessing the shared resource, the textFrameSender class variable.
Additionally, could you please clarify what specific scenarios you had in mind regarding 'competing send to undercut one another'?
Understanding this will help me consider those cases and make further improvements accordingly.
| } | ||
| } | ||
| partialByte.clear(); | ||
| } | ||
|
|
||
| private synchronized StreamSinkFrameChannel getTextFrameSender() throws IOException { |
There was a problem hiding this comment.
Two different senders are guarded by single instance lock. Is there a reason for such arrangement?
There was a problem hiding this comment.
@baranowb
Thank you for your question.
The textFrameSender and binaryFrameSender class variables are initialized and set to null during partial message transmission.
During this process, all other send() methods are blocked from sending messages. Specifically, while sending partial messages using textFrameSender, transmissions using binaryFrameSender are also restricted.
If we were to apply separate locks for textFrameSender and binaryFrameSender, there would be a high risk of deadlocks, especially when different methods interact with these variables concurrently.
To mitigate this, we decided to use a single instance lock to manage both senders.
Additionally, other send() methods call assertNotInFragment() to check if a partial message is being transmitted.
Using separate locks in this scenario could similarly lead to deadlocks.
For these reasons, we opted for a single lock to ensure safe and consistent behavior.
There was a problem hiding this comment.
@baranowb
Alternatively, it seems possible to control concurrency by declaring textFrameSender and binaryFrameSender as AtomicReference<StreamSinkFrameChannel> without using synchronized.
Hello.
I modified it in consideration of the case where NPE can occur when sending text message to websocket session.
Please check the UNDERTOW-2532 jira ticket for more information.
Thanks.