forkingPingThread vs. connection timeout

[twittner]

If the frequency of pings generated by forkingPingThread is higher than Snap's default timeout, the connection will stay alive even if clients never send anything back to the server. The reason for this is that every time a ping is send Snap will tickle the timeout (cf. SimpleBackend.hs, in particular the function writeEnd).

Instead it would be preferable to set the tickle timeout to a value less than the thread delay, for example:

diff --git a/src/Network/WebSockets/Snap.hs b/src/Network/WebSockets/Snap.hs
index 2ca4c67..773ac45 100644
--- a/src/Network/WebSockets/Snap.hs
+++ b/src/Network/WebSockets/Snap.hs
@@ -136,7 +136,7 @@ runWebSocketsSnapWith options app = do
             pc = WS.PendingConnection
                     { WS.pendingOptions  = options'
                     , WS.pendingRequest  = fromSnapRequest rq
-                    , WS.pendingOnAccept = forkPingThread
+                    , WS.pendingOnAccept = forkPingThread tickle
                     , WS.pendingIn       = is
                     , WS.pendingOut      = os
                     }
@@ -147,13 +147,14 @@ runWebSocketsSnapWith options app = do

 --------------------------------------------------------------------------------
 -- | Start a ping thread in the background
-forkPingThread :: WS.Connection -> IO ()
-forkPingThread conn = do
+forkPingThread :: ((Int -> Int) -> IO ()) -> WS.Connection -> IO ()
+forkPingThread tickle conn = do
     _ <- forkIO pingThread
     return ()
   where
     pingThread = handle ignore $ forever $ do
         WS.sendPing conn (BC.pack "ping")
+        tickle (min 15)
         threadDelay $ 30 * 1000 * 1000

     ignore :: SomeException -> IO ()

If clients should be given a second chance one could use a flag (or counter) between the ping thread and connectionOnPong and only reduce the tickle timeout on the second (or nth) attempt.

[jaspervdj]

Hey man, thanks for finding this issue. I thought about this for a while and I think your solution will solve the issue. Appreciate it! :+1:

Would it be possible for you send me a pull request for this? That way you get proper accreditation.

[twittner]

In addition to the above I propose to reset the timeout on incoming data (i.e. in copyIterateeToMVar). Otherwise clients which are busy sending data but are not replying with a pong will be disconnected.