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

Jump to bottom

Add ping and pong received callbacks #509

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Add ping and pong received callbacks #509

merged 1 commit into from
Jan 30, 2025
+135 −5

Conversation

igolaizola
Copy link
Contributor

@igolaizola igolaizola commented Jan 28, 2025 •
edited by mafredri
Loading

Add ping and pong received callbacks

This change adds two optional callbacks to both DialOptions and
AcceptOptions. These callbacks are invoked synchronously when a ping
or pong frame is received, allowing advanced users to log or inspect
payloads for metrics or debugging. If the callback needs to perform more
complex work or reuse the payload outside the callback, it is
recommended to perform processing in a separate goroutine.

The boolean return value of OnPingReceived is used to determine if the
subsequent pong frame should be sent. If false is returned, the pong
frame is not sent.

Tests confirm that the ping/pong callbacks are invoked as expected.

Fixes #246
Closes #307

@igolaizola igolaizola mentioned this pull request Jan 28, 2025
Closed
@igolaizola
Copy link
Contributor Author

It seems the tests I added are flaky. I'm investigating this.

@mafredri mafredri self-requested a review January 28, 2025 09:27
@igolaizola
Copy link
Contributor Author

It seems the tests I added are flaky. I'm investigating this.

I've found the problem. newConnTest returns the two connections in a random order. I'm fixing the test to account for this.

@igolaizola
Copy link
Contributor Author

pingPongReceived test is passing now, but it seems there is an unrelated test (TestWasm) that is failing here and in other PRs.

mafredri
mafredri reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! Looks great, I just had some questions/concerns about the ping received payload modifications.

mafredri
mafredri reviewed Jan 29, 2025
View reviewed changes
@igolaizola igolaizola force-pushed the ping-pong-callbacks branch 2 times, most recently from 38c82dd to a9a6a1f Compare January 29, 2025 12:09
mafredri
mafredri reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

This looks great and the docs are well written too, thanks! Once the data race in the tests is fixed, I believe this is good to go 👍🏻

@igolaizola
Add ping and pong received callbacks
Loading
Loading status checks…
85e8670 
This change adds two optional callbacks to both `DialOptions` and
`AcceptOptions`. These callbacks are invoked synchronously when a ping
or pong frame is received, allowing advanced users to log or inspect
payloads for metrics or debugging. If the callback needs to perform more
complex work or reuse the payload outside the callback, it is
recommended to perform processing in a separate goroutine.

The boolean return value of `OnPingReceived` is used to determine if the
subsequent pong frame should be sent. If `false` is returned, the pong
frame is not sent.

Tests confirm that the ping/pong callbacks are invoked as expected.

References coder#246
@igolaizola
Copy link
Contributor Author

igolaizola commented Jan 30, 2025 •
edited
Loading

It looks like we are now encountering an issue related to wasmbrowsertest and timeouts. I'm trying to update it to the latest version to see if that resolves the problem.

This repo had a similar issue, and it looks like they fixed it by updating the version as well: hajimehoshi/ebiten#2982

[edit] Moved the fix attempt to a new PR: #514

@igolaizola igolaizola mentioned this pull request Jan 30, 2025
Merged
@igolaizola
Copy link
Contributor Author

This looks great and the docs are well written too, thanks! Once the data race in the tests is fixed, I believe this is good to go 👍🏻

CI is passing now. Feel free to merge this.

We can discuss the flaky TestWasm here (#514), as it is unrelated to this PR change.

mafredri
mafredri approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the contribution!

@mafredri mafredri merged commit 703784f into coder:master Jan 30, 2025
7 of 8 checks passed
@igolaizola igolaizola deleted the ping-pong-callbacks branch January 30, 2025 12:22
@chrpwd
Copy link

chrpwd commented Feb 26, 2025 •
edited
Loading

Hi,

This is great. Any info on when the next release will be? Thanks to all the maintainers and contributors!

@mafredri

@mafredri
Copy link
Member

@chrpwd the release is out now v1.8.13! 🎉

@yaslama yaslama mentioned this pull request Mar 18, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mafredri mafredri

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add callbacks to log when Ping/Pong is received
3 participants
@igolaizola @chrpwd @mafredri