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

Websocket bug: PING to server sends PING Frame instead of PONG Frame #5533

Merged
merged 3 commits into from Mar 28, 2024

Conversation

srasul
Copy link
Contributor

@srasul srasul commented Mar 26, 2024

Motivation:

There is a bug in the Websocket Server PING/PONG handing. When a PING message is sent to Server, the Server incorrectly sends out another PING message as a reply. This is because the code to create the PONG message has a bug in it. The problem is that when testing the PING/PONG mechanism, the Server incorrectly send a PING as a reply for a PING. And this caused whoever was sending the initial PING to receive an incorrect reply to the PING. This was only happening for the case when the PING message has data.

Modifications:

  • In the method: com.linecorp.armeria.common.websocket.WebSocketFrame#ofPong(byte[]), there is the bug. I modified the code to return the correct Frame. See the files changed.
  • I also created unit test for this case

image

Result:

  • Websocker PING that contain data are now correctly handled and a PONG Frame sent out.
  • This means that when testing with a websocket cli tool, I now get the correct frame.
    BEFORE:
$ websocat --print-ping-rtts --ping-interval 5 ws://localhost:8080/ws
[WARN  websocat::ws_peer] Received a pong with a strange content from websocket
[WARN  websocat::ws_peer] Received a pong with a strange content from websocket
[WARN  websocat::ws_peer] Received a pong with a strange content from websocket

AFTER:

$ websocat --print-ping-rtts --ping-interval 5 ws://localhost:8080/ws
RTT 0.003123 s
RTT 0.000542 s
RTT 0.000530 s

…SocketFrameType

* added unit test that sends a PING from client to server, and server now returns the correct PONG Frame
@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@minwoox minwoox added the defect label Mar 27, 2024
@minwoox minwoox added this to the 1.28.0 milestone Mar 27, 2024
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.05%. Comparing base (0ab1721) to head (55a4733).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5533      +/-   ##
============================================
+ Coverage     74.03%   74.05%   +0.01%     
- Complexity    20854    20871      +17     
============================================
  Files          1807     1808       +1     
  Lines         76754    76808      +54     
  Branches       9790     9800      +10     
============================================
+ Hits          56827    56877      +50     
+ Misses        15301    15295       -6     
- Partials       4626     4636      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Oops... Nice catch! 🙇‍♂️🙇‍♂️

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@minwoox minwoox merged commit 146b1be into line:main Mar 28, 2024
18 checks passed
@srasul srasul deleted the fix_pong_WebSocketFrame branch April 15, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants