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.

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

Fix nullpointer on null code websockets-next #44480

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

Malandril
Copy link
Contributor

Fix nullpointer in ws-next basic connector when the websocket closeStatusCode is null.

@Malandril Malandril changed the title Fix nullpointer on null code ws-next Fix nullpointer on null code websockets-next Nov 13, 2024
@gsmet gsmet requested a review from mkouba November 14, 2024 08:54
@@ -239,7 +239,11 @@ public void handle(Void event) {
trafficLogger.connectionClosed(connection);
}
if (closeHandler != null) {
doExecute(connection, new CloseReason(ws.closeStatusCode(), ws.closeReason()), closeHandler);
int code = CloseReason.INTERNAL_SERVER_ERROR.getCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you actually observe a NPE in your app? Because the javadoc WebSocket.closeHandler(Handler<Void>) is pretty clear that "When the WebSocket received a close frame, the closeStatusCode() will return the status code and closeReason() will return the reason."

But maybe I just misundertood.

We could reuse the CloseReason.INTERNAL_SERVER_ERROR constant, i.e. something like:

CloseReason reason = CloseReason.INTERNAL_SERVER_ERROR;
if (ws.closeStatusCode() != null) {
   reason = new CloseReason(ws.closeStatusCode(), ws.closeReason());
}
doExecute(connection, reason, closeHandler);

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you actually observe a NPE in your app? Because the javadoc WebSocket.closeHandler(Handler<Void>) is pretty clear that "When the WebSocket received a close frame, the closeStatusCode() will return the status code and closeReason() will return the reason."

Yes for example closing a websocket server websocat with CTRL+C.
This is also being done in

@Malandril Malandril force-pushed the bugfix/nullpointer_websocket-next branch from 5725886 to 1069847 Compare November 14, 2024 10:08
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Thanks!

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 14, 2024
Copy link

quarkus-bot bot commented Nov 14, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 1069847.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@mkouba mkouba merged commit 5e84cc8 into quarkusio:main Nov 14, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 14, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 14, 2024
@mkouba mkouba added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport labels Nov 14, 2024
@gsmet gsmet modified the milestones: 3.18 - main, 3.16.4 Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants