search

faye / faye-redis-ruby

Redis engine backend for Faye
80 stars 38 forks source link

Bayeux reconnect advice #4

Closed mainameiz closed 11 years ago

mainameiz commented 11 years ago

According to 3.6.1. section of bayeux protocol specification 

3.6.1. reconnect advice

The reconnect advice field is a string that indicates how the client should act in the case of a failure to connect. Defined reconnect values are:

retry
    a client MAY attempt to reconnect with a /meta/connect after the interval (as defined by "interval" advice or client-default backoff), and with the same credentials.

Redis engine is garbage collecting clients that do not use reconnect. Is it possible to keep clients and do not make them reconnect? Because this reconnect messages are not really needed when transport is websocket (and it has ping messages itself).

jcoglan commented 11 years ago

I'm not entirely sure what you're asking to change, but here's a few things I think are relevant.

First, the engine layer is totally agnostic of the network transport layer between the server and the client. This greatly simplifies its implementation since the whole mess of network details is kept out of the engines, giving them a consistent set of semantics to build to regardless of the transport in use. This semantics has been designed to cover all possible transports and their capabilities in an abstract way.

Second, /meta/connect provides a place to hook in access-control and other extensions, in a way that's agnostic about the transport, which again conveys similar simplicity benefits on those writing extensions. Again, this is a case of the protocol covering the minimal semantics supportable by all transports in an abstract way.

Finally, /meta/connect is the means by which WebSocket connections identify themselves: WebSockets gain a clientId when a /meta/connect message is sent over the socket, identifying the client. Yes, we could make each WebSocket only send one such message, but this would introduce complexity when dealing with sockets that get reused with a new clientId, which happens when the server is restarted.

So, while what you seem to be proposing might work in the special case of RFC-compliant WebSockets that support PING frames, the amount of special casing involved is undesirable and makes transport concerns leak into areas where it would make maintenance harder.

Does this answer your question?

jcoglan commented 11 years ago

In addition, skipping sending /meta/connect when using a WebSocket transport would probably introduce incompatibilities with other Bayeux implementations, which many people use the Faye client to talk to. And there's not a lot of point changing our server if the client cannot take advantage of these changes.

mainameiz commented 11 years ago

Ok. I will try to describe a resolution. It's ok that engine is agnostic of the network layer. As I see in the code, engine deletes clients which last access time is greater then timeout (https://github.com/faye/faye-redis-ruby/blob/master/lib/faye/redis.rb#L182). It is possible "touch" them on server side (update last access time) if they use websockets? Not on the engine level (but using it).

UPD: engine already have ping method. Why not use it in a websocket transport (e.g. create timeout which will ping engine). So I want to replace the responsibility of clients to send /meta/connect to the server which will ping engine.

jcoglan commented 11 years ago

Sure, you could have the server-side WebSocket handler call ping() on the engine to keep the clientId alive, but I'm still not sold. Why are you so eager to have the client stop sending /meta/connect?

yvbeek commented 11 years ago

I'm going to chime in, for my iOS app I've created an Objective-C Faye client based on SocketRocket (websocket library), I was quite surprised that the server would disconnect the client after the server-side timeout.

The WebSocket layer will maintain a proper connection with the server. It seems wasteful to have to send a connect message every 30sec. to keep the connection alive. This should be the responsibility of the websocket layer.

jcoglan commented 11 years ago

I've checked this out and implementing your suggesting breaks compatibility with the reference implementation, so I'm closing the issue. Besides, it's really a very small bandwidth saving that would introduce complexity in several places.