Feedback suggestions on withPingPong API

[ulidtko]

Hi @jaspervdj! Huge thanks for maintaining this package.

I've got a Servant webserver with a Websocket endpoint built on top of this library.

Having noticed the deprecation on forkPingThread, I gave withPingPong a try.

Observed a few issues regarding API ergonomics — sharing, if that helps.

1. Generalize to MonadIO, please

First and most serious ask, is to generalize the signature:

withPingPong :: PingPongOptions -> Connection -> (Connection -> IO ()) -> IO () 
-- ↓↓↓
withPingPong :: MonadIO m
             => PingPongOptions -> Connection -> (Connection -> m ()) -> m () 

To motivate, let me sketch an example of a typical Servant server that includes a WS endpoint:

type MyApi = "example.post204" :> ReqBody '[OctetStream] LByteString :> PostNoContent
        :<|>  -- many other endpoints
        :<|> "evstream" :> WebSocket

server :: Server MyApi
server = handlerSample
    :<|> -- other handlers
    :<|> handleEventStream 

handlerSample :: LByteString -> Handler NoContent
handlerSample _reqbody = return NoContent

handleEventStream :: Network.Websockets.Connection -> Handler ()
handleEventStream c = do
  liftIO $ forkPingThread c 10
  liftIO . forM_ [1..] $ \i -> do
    sendTextData c (pack $ show (i :: Int)) >> threadDelay 1000000

Even without going into custom monad stacks (which is fairly common to do, too) — notice that, in the vanilla servant textbook setup, endpoint handlers like handlerSample run in the Handler monad, not IO directly — including Websocket endpoints.

The Handler is of course MonadIO (just like a custom stack with logging, config Reader, metrics, etc, would be) — so it's trivial to liftIO simple IO () actions like forkPingThread into it.

It stops being trivial when the IO appears in both contravariant & covariant positions — like in withPingPong signature.

That can be worked around, nontrivially. By employing unliftio, I can write an orphan instance that reaches into Handler guts:

instance MonadUnliftIO Servant.Handler where
  withRunInIO :: ((forall x. Handler x -> IO x) -> IO y) -> Handler y
  withRunInIO wrapped = Handler . ExceptT $ do
    let runner = runHandler >=> either throwIO return
    (Right <$> wrapped runner) `catch` (return . Left @ServerError)

Then, I can wrap withPingPong to run my handler in whatever monad I need, be it Handler or a custom transformer stack WebM, as long as it's MonadUnliftIO:

websocketCmdChan :: WS.Connection -> WebM ()
websocketCmdChan wsconn
  = withWebsocketKeepAlive wsconn
  $ forever
  $ do
    -- WS.receiveData wsconn
    -- ...
    -- WS.sendTextData wsconn ...
    -- ...
  where
    withWebsocketKeepAlive :: MonadUnliftIO m => WS.Connection -> m () -> m ()
    withWebsocketKeepAlive conn action
      = askRunInIO >>= \runInIO -> liftIO $
        WS.withPingPong WS.defaultPingPongOptions conn (const $ runInIO action)

... which is quite a hassle just to use a library function to get websocket keepalives, right?

The whole workaround can be abolished, the restriction to MonadUnliftIO can be avoided (relaxed to only MonadIO), the orphan instance & the extra dependency can be dropped, and the library function can be made to Just Work™ in less-trivial cases — by generalizing its signature to MonadIO like suggested above.

2. The Connection argument is awkward

The next suggestion is minor — its respective workaround is a const call — to change the signature of withPingPong like so:

withPingPong :: PingPongOptions -> Connection -> (Connection -> IO ()) -> IO () 
-- ↓↓↓
withPingPong :: PingPongOptions -> Connection -> IO () -> IO ()

The signature as it stands, suggests CPS style, misleadingly, because the wrapped action is not actually used as a continuation.

I also don't see how, logically, the downstream Connection (that the wrapped action accepts) could be different from the Connection passed to withPingPong. Indeed, the current implementation just passes it straight through. Is there an idea that these 2 may somehow end up being different?..

The lexical scope that invokes withPingPong must have the connection (otherwise the function can't be called); and conceivably, the wrapped action can often be an inline do-block — having access to the same connection in the same scope.

I mean, here, there's no point to demand \wsconn' ->, as we still have wsconn in scope:

websocketCmdChan :: WS.Connection -> WebM ()
websocketCmdChan wsconn
  = withWebsocketKeepAlive wsconn
  $ forever
  $ do
    -- WS.receiveData wsconn
    -- ...
    -- WS.sendTextData wsconn ...
    --                 ^^^^^^
    --                 exists

3. Ditto for withPingThread

As you probably know well enough already, not all WS clients react to Ping, as they must per the standard. Turns out, I'm currently working with one of those. Regardless of not receiving Pong's, I still want to send periodic Ping's to keep-alive the socket (as it gets proxied). AFAICS, withPingThread is provided for that exactly.

It has the same MonadIO issue as withPingPong however, so that suggestion still applies.

[jaspervdj]

Hi @ulidtko, thanks for the feedback!

---

I somewhat agree with (2) -- the connection argument is just passed through. However, I can see two reasons to do this:

1. This feels somewhat consistent with the type ClientApp a = Connection -> IO a
2. It allows us to tweak fields on the connection (e.g. set a reference to the ping/pong settings)

---

I am not sure about (1) though. Since we are using withAsync, I think we cannot get away with just a MonadIO constraint (which introduces an mtl dependency), we also need to add a MonadUnliftIO constraint (which introduces an unliftio-core dependency), because IO occurs in a negative/argument position.

This means that the MonadUnliftIO instance for e.g. Handler needs to be written regardless of whether or not we offer a generic API. And once that instance is written, offering the generic API is just an almost-one-liner away:

withPingPong'
    :: MonadUnliftIO m
    => PingPongOptions -> Connection -> (Connection -> m ()) -> m ()
withPingPong' opt conn app =
    withRunInIO $ \run -> withPingPong opt conn (run . app)

It is debatable but IMO introducing two new dependencies and committing to Monad Transformers seems a bit heavy-handed for this library which tries to offer a low-level API (like withFile from System.IO), if the alternative is just having a version of this one-liner in the user's code? I'm happy to improve the documentation around this though.

Curious to hear @domenkozar's thoughts as well.

[domenkozar]

1) I'd be happy to support unliftio-core as I use it myself, but either way we should document this.

2) Sounds good to me.

[ulidtko]

Ohhh I see, withAsync basically forces MonadUnliftIO — so I was wrong, generalizing to MonadIO only won't help.

Agree that writing out the relevant MonadUnliftIO instance is on the user.

This feels somewhat consistent with the type ClientApp a = Connection -> IO a

Yep; had another thought in this direction, but initially omitted to reduce clutter:

withPingPong :: PingPongOptions -> (Connection -> IO ()) -> Connection -> IO ()

Might compose better in point-free style, because PingPongOptions argument makes using flip a little bit unwieldy/confusing:

myHandler :: WS.Connection -> WebM ()
myHandler
  = flip (withPingPongU defaultPingPongOptions) $ \conn -> do
    {- ... -}

BTW, there's this convention, a U suffix for "unlifted" versions of IO-only functions — met it several times, and IMO it's intuitive enough.

I'd say, it'd be super-convenient if websockets package provided unlifted withPingPongU, withPingThreadU functions! Perhaps a sub-package websockets-unlifted? Perhaps a cabal-flag?..

If all else fails, maybe just a snippet in docs: (modulo reordering of arguments, if any)

withPingPongU
    :: MonadUnliftIO m
    => PingPongOptions -> Connection -> (Connection -> m ()) -> m ()
withPingPongU opt conn app =
    withRunInIO $ \run -> withPingPong opt conn (run . app)

withPingThreadU
    :: MonadUnliftIO m
    => Connection -> Int -> m () -> m a -> m a
withPingThreadU conn n action app =
    withRunInIO $ \run -> withPingThread conn n action (run app)