skip handshake if using websocket as transport. #197
skip handshake if using websocket as transport. #197adamzul wants to merge 5 commits intoWisembly:masterfrom
Conversation
…s use full if socket server multi node and not stick.
Taluu
left a comment
Taluu
left a comment
There was a problem hiding this comment.
I'm uneasy on how this is done, especially the part on upgradeTransport (it should do nothing if we're already on websocket).
Are we sure there's no handshakes to be made first ?
|
i get problem if my socket server using multi node and using polling transport, which need server to make session id. but session id only recognized by the node that created it. so then i try using websocket transport. as far as i know if we use websocket , we not using handshake . In current code, it force to make handshake and make session from server |
| } | ||
|
|
||
| $this->handshake(); | ||
| if($this->options['transport'] != 'websocket'){ |
There was a problem hiding this comment.
Could you make coding style consistency? And this code snippet should be:
......
if ($this->options['transport'] != 'websocket') {
......There was a problem hiding this comment.
give space between ')' and '{' ?
There was a problem hiding this comment.
@Taluu, maybe it should consider some coding style to check during Travis CI build.
We can consider using PHP_CodeSniffer or PHP-CS-Fixer.
Do you have any idea about this?
There was a problem hiding this comment.
Yup, adding this (with maybe building through GA instead of Travis) would be awesome. Not sure I have the time for that currently though, especially as I'm not really maintaining it (because not using it...) currently...
| { | ||
| if ($this->session->needsHeartbeat()) { | ||
| $this->write(static::PING); | ||
| if($this->session){ |
There was a problem hiding this comment.
It should be:
......
if ($this->session) {
......There was a problem hiding this comment.
Even better :
if ($this->session === null) {
return;
}
// ...There was a problem hiding this comment.
But then we can't keep alive the connection, it will be closed at some point.
There was a problem hiding this comment.
or maybe we can by pass run write if session null?
| protected function upgradeTransport() | ||
| { | ||
| $query = ['sid' => $this->session->id, | ||
| $query = ['sid' => isset($this->session->id) ? $this->session->id : null, |
There was a problem hiding this comment.
It seems that the package requires php-7.1 version at least.
And it can consider using the ?? syntax:
......
$this->session->id ?? null,
......
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
My bad, didn't remember it was bumped 10 months ago. 😂
(We should still bump to maintained php versions though)
|
Any status update here ? I'm okay for allowing to "skip" the handshake if needed (didn't verify, I trust you guys :P). |
3 participants
websocket transport is use full if socket server multi node and not stick.