centrifugal / centrifuge-dart

Dart (Flutter) client SDK for bidirectional communication with Centrifugo and Centrifuge-based server over WebSocket
https://pub.dartlang.org/packages/centrifuge
MIT License
102 stars 29 forks source link

Add ping/pong method #39

Closed Holofox closed 4 years ago

Holofox commented 4 years ago

I did not notice a ping-pong implementation that would work. At the moment, the socket connection is broken after a timeout with ping-pong, so I decided to implement a working ping-pong.

FZambia commented 4 years ago

Hello, thanks!

At the moment, the socket connection is broken after a timeout with ping-pong, so I decided to implement a working ping-pong

So do you have a case where you proved that non-websocket ping fixed connection break? I am not saying this is a surprise for me - just want to get more information on the story behind.

Skarm commented 4 years ago

Hello, thanks!

At the moment, the socket connection is broken after a timeout with ping-pong, so I decided to implement a working ping-pong

So do you have a case where you proved that non-websocket ping fixed connection break? I am not saying this is a surprise for me - just want to get more information on the story behind.

Hello. The client disconnects if tries to send ping (if do not set the setting ping interval to null ). there is no proto ping implementation in client. Native ping is sent with an empty message. Therefore, the centrifuge breaks the connection:

if len(data) == 0 {
c.node.logger.log(newLogEntry(LogLevelError, "empty client request received", map[string]interface{}{"client": c.ID(), "user": c.UserID()}))
....

https://github.com/centrifugal/centrifuge/blob/master/client.go#L534-L538

FZambia commented 4 years ago

Native ping is sent with an empty message. Therefore, the centrifuge breaks the connection

Native pings sent as special WebSocket control frames, server does not explicitly processes them.

If it worked the way you said then I suppose it will be easily reproduced using console example in this repo (it does not). I suppose @Holofox means some problems with intermediaries that lead to connection problems.

Skarm commented 4 years ago

Native ping is sent with an empty message. Therefore, the centrifuge breaks the connection

Native pings sent as special WebSocket control frames, server does not explicitly processes them.

If it worked the way you said then I suppose it will be easily reproduced using console example in this repo (it does not). I suppose @Holofox means some problems with intermediaries that lead to connection problems.

Yep, "%x9" should be sent, but no :) Tested in a centrifuge (+gobwas/ws), gave an error on ping: "empty client request received".

FZambia commented 4 years ago

@Skarm I suppose that the reason is that example which uses gobwas/ws just demonstrates possibility to work with that library but it does not properly handle native WebSocket ping/pong frames. Could you try the same without custom transport implementation or with Centrifugo?

Skarm commented 4 years ago

@FZambia Of course, this example is used inaccurately in the presented form, he rewritten.

But even if test the presented example, then in js client pings work:

Screenshot 2020-06-06 at 19 08 25

what am I doing wrong?:)

I can’t check the dart client yet, maybe later.

FZambia commented 4 years ago

@Skarm just fixed gobwas/ws example to properly handle pings, as I said the reason was in incomplete example implementation. Take a look at https://github.com/centrifugal/centrifuge/commit/42cec58b653aa7184508191776c9096bbccc3096

I don't understand - are you working together with @Holofox or you just mentioned similar problem you came across on your own?

Holofox commented 4 years ago

Yes, we work together on one issue. Initially, I thought that ping on web sockets implicitly did not work, because the timeout expired and the connection was reset, so I implemented an explicit ping/pong, where everything also works, now an explicit implementation is no longer required.

@FZambia, thank you very much for clarifying the situation in detail, I think the problem is solved.

FZambia commented 4 years ago

I suggest to stay with Centrifuge core and not go to custom WebSocket transport implementation without reason. You win pretty much in memory usage with gobwas/ws but you should have a really good understanding on underlaying mechanisms used there to avoid problems in production in future. You can reduce Websocket buffers of builtin transport implementation to reduce memory usage a bit. For example set ReadBufferSize and WriteBufferSize to 512 like I did in benchmark