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

Client Ping/Pong broken in 1.6 #170

Open
MarcusOhman91 opened this issue Nov 8, 2022 · 5 comments
Open

Client Ping/Pong broken in 1.6 #170

MarcusOhman91 opened this issue Nov 8, 2022 · 5 comments
Labels

Comments

@MarcusOhman91
Copy link

MarcusOhman91 commented Nov 8, 2022

Pong request payload does not match ping response payload (which causes the websocket server I try to interact with to close the connection)

In my scenario the ping from the websocket server is an empty string. When producing the pong response, the pushFrame is invoked receiving the following $frame argument: [true,"","pong",false]. This in turn produces a $data of �\u0000" which is written to the stream.

I could quickfix it for my scenario by changing this line
$this->write($data);
to
$this->write($opcode === 'pong' ? '' : $data);

But I instead solved it by downgrading to v1.5.8 where the ping/pong interaction seems to work fine.

Reproduced with PHP 8.1.12 (cli)
version 1.6.3

@sirn-se
Copy link
Contributor

sirn-se commented Nov 8, 2022

Did some checks, and the autoresponder do reply as expected on empty message. The issue you have is probably related to masking. This has been wrong all along, but it appears my attempt to improve things in v1.6 caused other issues instead. I'll have a look at that when I've got some time for it.

As a side note, the quick fix you mention means the client do not send a pong when receiving ping at all. The $data variable is the entire frame, not only the payload.

@sirn-se
Copy link
Contributor

sirn-se commented Nov 12, 2022

Hi @MarcusOhman91

I did a rewrite of masking policy that (hopefully) will solve the issue you're experiencing; #171

However, the servers I'm testing with work with v1.6 as is. So I would appreciate if you're able to confirm that this fix solves things for you before I merge it to master. You should be able to test by specifying dev-1.6-fixes in Composer.

Thanks in advance!

@sirn-se
Copy link
Contributor

sirn-se commented Nov 22, 2022

@MarcusOhman91
Please switch to phrity/websocket (official fork of this repo), version 1.6.4 should solve your issue.

@thorewi
Copy link

thorewi commented Jan 1, 2023

The PR was closed and not merged, why?

@MarcusOhman91
Copy link
Author

@MarcusOhman91 Please switch to phrity/websocket (official fork of this repo), version 1.6.4 should solve your issue.

delayed response, but can confirm that this worked.

I guess this issue should be left open if any of the remaining maintainers decides to fix it in this repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants