search

dariocravero / padrino-websockets

Agnostic websockets support for Padrino
MIT License
14 stars 9 forks source link

problem in EventManager #5

Closed janus112 closed 9 years ago

janus112 commented 9 years ago

Hello, I think that you have a problem in EventManager, which implies a user is not removed from a channel, if the connection is closed.

Since you define on_shutdown(event) in EventManager, its not called in BaseEventManager.

I would remove it, since Faye takes care of removing the pinger, and let BaseEventManager define on_shutdown(event), that is, appending event argument.

In BaseEventManager, I guess on_shutdown(event) should call @@connections[@channel].delete(@user)

and not

@@connections[@channel].delete(@ws)

dariocravero commented 9 years ago

Hi @janus112, thanks for your comment and spotting that!

Perhaps we could replace this with:

def on_shutdown(event)
  @pinger.cancel if @pinger
  super
end

The other fix you're pointing to is correct too.

Can you verify that that #6 does it?

janus112 commented 9 years ago

Its indeed working. Great. Ill be using this in production soon.

dariocravero commented 9 years ago

Merged! I'll push it to rubygems later on today. Sorry about that, even though it works, this gem clearly needs unit testing; I never got around that and I don't think it will happen anytime soon unfortunately. PRs welcome :).

janus112 commented 9 years ago

Sounds great. No problem. Even if it was unit tested, I would review it anyway for production. Thanks

Den 22/04/2015 kl. 12.11 skrev Darío Javier Cravero notifications@github.com:

Merged! I'll push it to rubygems later on today. Sorry about that, even though it works, this gem clearly needs unit testing; I never got around that and I don't think it will happen anytime soon unfortunately. PRs welcome :).

— Reply to this email directly or view it on GitHub https://github.com/dariocravero/padrino-websockets/issues/5#issuecomment-95116557.

dariocravero commented 9 years ago

@janus112 version released!