jaspervdj / websockets-snap

Snap integration for the websockets library
Other
33 stars 21 forks source link

pendingOnAccept callback be optional? #5

Closed banacorn closed 11 years ago

banacorn commented 11 years ago

Hi again! :)

I've noticed that there's a forkPingThread callback attached to the field pendingOnAccept, and I ported it to wai-websockets, too, because I thought it was essential.

-- websockets-snap-0.8.0.0
-- Network/WebSockets/Snap.hs L150
let pc = WS.PendingConnection
                   { WS.pendingOptions  = options'
                   , WS.pendingRequest  = fromSnapRequest rq
                   , WS.pendingOnAccept = forkPingThread
                   , WS.pendingIn       = is
                   , WS.pendingOut      = os
                   }

But then I came across websockets-0.8.0.0 and found it left out like this.

-- websockets-0.8.0.0
-- Network/WebSockets/Server.hs L73
let pc = PendingConnection
                { pendingOptions  = opts
                , pendingRequest  = request
                , pendingOnAccept = \_ -> return ()
                , pendingIn       = sIn
                , pendingOut      = bOut
                }

Is forkPingThread a snap-related hack and should I remove it from wai-websockets?

Are those fields meant to be internal stuff because I can't seem to configure them (as an end user).

jaspervdj commented 11 years ago

The reason this ping thread is spawned for websockets-snap is that Snap kills the connection after a set amount of seconds (I think 30) unless the connection has been "tickled" (e.g. by sending a ping). This is a built-in protection against certain types of DDoS attacks. If Warp doesn't kill the connection, I don't think a ping thread is needed by default.

banacorn commented 11 years ago

Got it! Thanks!