Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UNDERTOW-2532] Fix NPE when transmitting text or binary message to websocket session at the same time #1708

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[UNDERTOW-2532] Fix NPE when transmitting text or binary message to websocket session at the same time #1708

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a105d1
[UNDERTOW-2532] Modify it to be thread safe when transmitting text or…
raccoonback Nov 29, 2024
77fd1aa
[UNDERTOW-2532] fix typo
raccoonback Dec 3, 2024
ccc3ae5
[UNDERTOW-2532] prevents intrinsic lock from being affected when tran…
raccoonback Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 40 additions & 33 deletions websockets-jsr/src/main/java/io/undertow/websockets/jsr/WebSocketSessionRemoteEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,10 @@ class BasicWebSocketSessionRemoteEndpoint implements Basic {
private StreamSinkFrameChannel textFrameSender;

public void assertNotInFragment() {
if (textFrameSender != null || binaryFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
synchronized (this) {
if (textFrameSender != null || binaryFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
}
}
}

Expand Down Expand Up @@ -273,50 +275,55 @@ public void sendText(final String partialMessage, final boolean isLast) throws I
if(partialMessage == null) {
throw JsrWebSocketMessages.MESSAGES.messageInNull();
}
if (binaryFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
}
if (textFrameSender == null) {
textFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.TEXT);
}
try {
Channels.writeBlocking(textFrameSender, WebSocketUtils.fromUtf8String(partialMessage));
if(isLast) {
textFrameSender.shutdownWrites();

synchronized (this) {
if (binaryFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
}
if(textFrameSender == null) {
textFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.TEXT);
}
Channels.flushBlocking(textFrameSender);
} finally {
if (isLast) {
textFrameSender = null;
try {
Channels.writeBlocking(textFrameSender, WebSocketUtils.fromUtf8String(partialMessage));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropalka That's a good comment! 👍
I will change the section so that there is no impact on the intrinsic lock.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ropalka
As you mentioned, the section "Channels.writeBlocking() ~ Channels.flushBlocking()" was excluded from the intrinsic lock range.

Please review again.

if(isLast) {
textFrameSender.shutdownWrites();
}
Channels.flushBlocking(textFrameSender);
} finally {
if(isLast) {
textFrameSender = null;
}
}
}

}

@Override
public void sendBinary(final ByteBuffer partialByte, final boolean isLast) throws IOException {

if(partialByte == null) {
throw JsrWebSocketMessages.MESSAGES.messageInNull();
}
if (textFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
}
if (binaryFrameSender == null) {
binaryFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.BINARY);
}
try {
Channels.writeBlocking(binaryFrameSender, partialByte);
if(isLast) {
binaryFrameSender.shutdownWrites();

synchronized (this) {
if (textFrameSender != null) {
throw JsrWebSocketMessages.MESSAGES.cannotSendInMiddleOfFragmentedMessage();
}
if (binaryFrameSender == null) {
binaryFrameSender = undertowSession.getWebSocketChannel().send(WebSocketFrameType.BINARY);
}
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, see comment above.

Channels.writeBlocking(binaryFrameSender, partialByte);
if (isLast) {
binaryFrameSender.shutdownWrites();
}
Channels.flushBlocking(binaryFrameSender);
}
Channels.flushBlocking(binaryFrameSender);
} finally {
if (isLast) {
binaryFrameSender = null;
finally {
if (isLast) {
binaryFrameSender = null;
}
}
partialByte.clear();
}
partialByte.clear();
}

@Override
Expand Down