fpco / streaming-commons

Common lower-level functions needed by various streaming data libraries
MIT License
36 stars 39 forks source link

Should `runTCPServer` / `runTCPServerWithHandle` return `Void`? #48

Open bitemyapp opened 6 years ago

bitemyapp commented 6 years ago
runTCPServerWithHandle :: ServerSettings -> ConnectionHandle -> IO a
runTCPServerWithHandle (ServerSettings port host msocket afterBind needLocalAddr _) handle =
    case msocket of
        Nothing -> E.bracket (bindPortTCP port host) NS.close inner
        Just lsocket -> inner lsocket
  where
    inner lsocket = afterBind lsocket >> forever (serve lsocket)
    serve lsocket = E.bracketOnError
        (acceptSafe lsocket)
        (\(socket, _) -> NS.close socket)
        $ \(socket, addr) -> do
            mlocal <- if needLocalAddr
                        then fmap Just $ NS.getSocketName socket
                        else return Nothing
            _ <- E.mask $ \restore -> forkIO
               $ restore (handle socket addr mlocal)
                    `E.finally` NS.close socket
            return ()

Quite possible I am wrong but IO a seems misleading to me.

/cc @mgsloan

mgsloan commented 6 years ago

In favor of this :+1: - see https://github.com/commercialhaskell/rio/issues/28

snoyberg commented 6 years ago

And from that issue, you can tell that I'm -1. I'd be happier with void instead of Void. I've tried hard to avoid breaking changes in this library, and don't see this as the time to make them.

bitemyapp commented 6 years ago

Even just the type variable name and a comment would help. I'd like to rip the bandaid off on this in the future if there's an opening.

snoyberg commented 6 years ago

Here's my article on the topic: https://www.fpcomplete.com/blog/2017/07/to-void-or-to-void. I don't think there should be a bandaid ripping off moment. I think void is the correct solution in positive position, and Void in negative position. I've heard the counterarguments, but they don't hold with me. I don't find being forced to sprinkle absurd through a code base adds meaningful type safety, and does add burden to a user of a library who probably just wanted the type to unify to (). So PR welcome for the change to void with more docs, but I'm not on board with a plan for a future move to Void.