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

Heartbeat on websocket #7

Closed
edevil opened this issue Jun 23, 2016 · 13 comments
Closed

Heartbeat on websocket #7

edevil opened this issue Jun 23, 2016 · 13 comments

Comments

@edevil
Copy link

edevil commented Jun 23, 2016

I've noticed that the JS phoenix code for sockets send a periodic heartbeat message to check if the socket is still healthy (and it probably helps to keep it that way).

I don't think this module does that at the moment, but it would be a nice to have feature.

@fbonetti
Copy link
Owner

The heartbeat code in phoenix.js checks to see if the websocket connection is still open. It if it's not, it attempts to reconnect using an exponential backoff strategy.

https://github.com/phoenixframework/phoenix/blob/master/web/static/js/phoenix.js#L452-L454

The elm-lang/websocket library already does this for us, so no extra code is needed. It even uses a similar exponential backoff strategy.

http://package.elm-lang.org/packages/elm-lang/websocket/1.0.1/WebSocket#listen

@edevil
Copy link
Author

edevil commented Jun 24, 2016

It doesn't just "check if the connection is open", it actually sends heartbeat messages.

Here on the JS side: https://github.com/phoenixframework/phoenix/blob/master/web/static/js/phoenix.js#L610

And here on the server side they are echoed:
https://github.com/phoenixframework/phoenix/blob/master/lib/phoenix/socket/transport.ex#L221

And in practice this helps keep the connection healthy. Connections from phoenix.js usually stick around while connections done with elm-phoenix-socket don't last very long for me, when they are idle.

@fbonetti fbonetti reopened this Jun 24, 2016
@fbonetti
Copy link
Owner

Yeah, you're right. The heartbeat instructs the phoenix server to keep the connection open, correct? This should be pretty easy to add. It looks like the heartbeat is sent from the client every 30 seconds. We could add a recursive Cmd that sends a heartbeat and then queues itself back up. I'm not sure if we need the exponential backoff strategy though.

@hojgr
Copy link

hojgr commented Jun 24, 2016

Edevil, the reason your elm-phoenix-socket don't last very long for you is due to Phoenix configuration. By default, Phoenix closes websocket after not recieving a message for 60 seconds - https://github.com/phoenixframework/phoenix/blob/master/lib/phoenix/transports/websocket.ex#L45

Ony way to go around it now is to set the :timeout to :infinity.

I also think that adding a heartbeat to this package is a good idea. I think there is a reason for the heartbeat to exist.

I don't think an exponential backoff strategy is needed because Elm's Websocket does that, as you said.

@edevil
Copy link
Author

edevil commented Jun 25, 2016

@hojgr @fbonetti Shouldn't Elm's Websocket reconnect in this case? Because I don't see it recover.

@prasmussen
Copy link

I was having the same issue mentioned in #5. I solved it by sending a heartbeat from elm to phoenix every 30 seconds. I guess this could be handled by the library. My solution:

subscriptions : Model -> Sub Msg
subscriptions model =
  Sub.batch
    [ Time.every (Time.second * 30) Heartbeat
    , Phoenix.Socket.listen model.socket PhoenixMsg
    ]

type Msg| Heartbeat Time

update : Msg -> Model -> (Model, Cmd Msg)
update msg model =Heartbeat _ ->
      let
        push =
          Phoenix.Push.init "heartbeat" "phoenix"
        (socket, cmd) =
          Phoenix.Socket.push push model.socket
      in
        ({model | socket = socket}, Cmd.map PhoenixMsg cmd)

@hojgr
Copy link

hojgr commented Jun 25, 2016

Edevil, I'm not sure what's your use case. I think it should reconnect. In my use case, the Elm client was only listening. So there was no impulse to reconnect (because Elm didn't know the socket is dead)

@igoodrich
Copy link
Contributor

igoodrich commented Jun 27, 2016

Excited about this library!

I think this library should mimic phoenix.js heartbeats as it is the stated intention of the library to do work like phoenix.js, it will yield the least surprising behavior, and it will enable this library to work with phoenix out of the box. Right now it really is broken out of the box without a heartbeat (see my more detailed description below). I may stick @prasmussen 's work into a PR if I'm feeling fine and frisky and full of time.

Here's exactly what I observed. My little elm program connected to phoenix, joined a channel, and on join, on the phoenix side, I started sending a ping message to the client every 5 seconds. Even though the client was happily receiving these ping messages, once every 5 seconds, after 60 seconds, the connection stopped (I think it was terminated from the phoenix side), elm reconnected (I think), and, for sure, I got a new socket on the phoenix side that was NOT joined to any channels. Consequently, the client simply stopped receiving ping messages. I'm not sure if a socket or process was leaked on the phoenix side, but it is a distinct possibility.

My conclusion is that w/o pushing data to the server, the web socket will be closed and all state on the socket will be lost.

Setting timeout to infinity may fix the problem in development, but it seems like that will likely cause resource issues in a production environment. I would say this is not a critical issue as there is a workaround, but until the documentation is fixed and/or a heartbeat is put in place, adoption of this library will likely be slowed. Still! Awesome work! Excited! 👍

@edevil
Copy link
Author

edevil commented Jun 27, 2016

elm reconnected (I think), and, for sure, I got a new socket on the phoenix side that was NOT joined to any channels

Yeah, this is also an issue. #9

@igoodrich
Copy link
Contributor

FYI: I think this resolves (at least it does for me):
#10

@fbonetti
Copy link
Owner

fbonetti commented Jul 5, 2016

@igoodrich I merged your PR into master and published a new version of the library to Elm Packages. I originally wanted this change to be a minor release, but the elm CLI tool elm package diff insisted that this was a major release 💩

------ Changes to module Phoenix.Socket - MAJOR ------

    Added:
        withHeartbeatInterval : Float -> Phoenix.Socket.Socket msg -> Phoenix.Socket.Socket msg
        withoutHeartbeat : Phoenix.Socket.Socket msg -> Phoenix.Socket.Socket msg

    Changed:
      - type alias Socket msg =
            { path : String,
              debug : Bool,
              channels : Dict.Dict String (Phoenix.Channel.Channel msg),
              events : Dict.Dict (String, String) (Json.Encode.Value -> msg),
              pushes : Dict.Dict Int (Phoenix.Push.Push msg),
              ref : Int
            }
      + type alias Socket msg =
            { path : String,
              debug : Bool,
              channels : Dict.Dict String (Phoenix.Channel.Channel msg),
              events : Dict.Dict (String, String) (Json.Encode.Value -> msg),
              pushes : Dict.Dict Int (Phoenix.Push.Push msg),
              ref : Int,
              heartbeatIntervalSeconds : Float,
              withoutHeartbeat : Bool
            }

@fbonetti fbonetti closed this as completed Jul 5, 2016
@igoodrich
Copy link
Contributor

@fbonetti That's kind of a bummer. It's API compatible with the prior version. I wonder if it's an issue with the CLI tool. Sorry for the trouble! And thanks for merging!

@fbonetti
Copy link
Owner

fbonetti commented Jul 5, 2016

Yeah, it seems API compatible to me as well, but the Socket record type is a part of the public API. Since the Socket record now has new fields, functions that take a Socket may no longer work with some records.

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

No branches or pull requests

5 participants