Remove EOF errors from receive functions #360

eborden · 2018-11-26T16:53:57Z

I'm opening this as an RFC. Please let me know what your thoughts are.

It is generally accepted that EOF errors in Network were a mistake.
Using exceptions for control flow is an anti-pattern in the Haskell
community. While we are in the process of introducing epoch breaking
changes it makes sense to attempt to remove these historic accidents.

There should really be no perceived change for users of
Network.ByteString(.Lazy).recv, however recvFrom does change
behavior. There are two key differences in recvFrom:

It is now returning a (0, address) on an EOF.
It will peek the socket's address and getPeerName on an EOF. This
has some implications for performance, and such may be controversial.

The second point poses a question. Does the return type of
SocketAddress sa => IO (Int, sa) make sense for recvBufFrom?

It is generally accepted that EOF errors in `Network` were a mistake. Using exceptions for control flow is an anti-pattern in the Haskell community. While we are in the process of introducing epoch breaking changes it makes sense to attempt to remove these historic accidents. There should really be no perceived change for users of `Network.ByteString(.Lazy).recv`, however `recvFrom` does change behavior. There are two key differences in `recvFrom`: 1. It is now returning a `(0, address)` on an EOF. 2. It will peek the socket's address and `getPeerName` on an EOF. This has some implications for performance, and such may be controversial. The second point poses a question. Does the return type of `SocketAddress sa => IO (Int, sa)` make sense for `recvBufFrom`?

Mistuke · 2018-11-26T21:45:11Z

+1 from me on removing the eof, as you say I'm not so sure about the result type of recvBufFrom either. Could it maybe be just IO Int? If the user wants the address they already have the Socket and can call getPeerName if needed.

eborden · 2018-11-26T23:41:08Z

I'm going to do a quick evaluation of source on hackage to see how frequently recvFrom and recvBufFrom are used. I have a sneaking suspicion that they are used rarely.

kazu-yamamoto · 2018-11-27T01:51:34Z

+1 from me, too.

eborden · 2018-11-27T15:00:15Z

Here is a rough accounting of calls on hackage:

`recvBufFrom`

tftp-0.2/src/Network/TFTP/UDPIO.hs:87:  mResult <- timeout timeoutMicros (Sock.recvBufFrom sock buffer bufferSize)
BufferedSocket-0.2.1.0/Network/BufferedSocket/Core.hs:218:                        (recievedBytes, _) <- NS.recvBufFrom sock offsetBuf maxRead

`recvFrom`

ntp-control-0.1/Network/NTP/Control.hs:24:  (b, sa) <- N.recvFrom s 1024
libjenkins-0.8.4/src/Jenkins/Discover.hs:70:readAnswer s = fst <$> ByteString.recvFrom s 4096
blubber-server-0.0.1/src-exec/blubber-server.hs:248:  (m, c) <- recvFrom sock 4096
zre-0.1.0.1/src/Network/ZRE/Beacon.hs:28:        (msg, addr) <- recvFrom sock 22
kademlia-1.1.0.0/src/Network/Kademlia/Networking.hs:89:        (received, addr) <- S.recvFrom (kSock kh) 1500
network-dns-1.1.0.1/examples/Resolver.hs:52:      (_, respBs) ← recvFrom sk 1024
mvclient-0.4/src/Network/Metaverse/Circuit.hs:463:recvRaw sock = fmap (Just . first deserialize) (recvFrom sock 10000)
TORCS-0.1.0.2/TORCS/Connect/Runner.hs:66:  (msg,addr) <- recvFrom conn 1024
TORCS-0.1.0.2/TORCS/Connect/Runner.hs:100:  (msg,d) <- catch (recvFrom conn 1024) (\(e :: SomeException) -> return ("",SockAddrUnix "")) --if nothing to sense from, get default value
datadog-0.2.2.0/test/Test/Network/Datadog/StatsD.hs:36:      val <- timeout 10000000 $ recvFrom conn 2048
conduit-extra-1.3.0/Data/Conduit/Network/UDP.hs:34:        (bs, addr) <- lift $ liftIO $ recvFrom socket len
tremulous-query-1.0.7/Network/Tremulous/Polling.hs:66:            packet <- ioForce finished $ recvFrom sock mtu
secureUDP-0.1.1.3/SecureUDP.hs:163:    (bString,sAddr) <- B.recvFrom (socket chcfg) (maxPacketSize chcfg + 4)
iterIO-0.2.2/Data/IterIO/Extra.hs:172:    genRecvFrom s len    = do (str, a) <- S.recvFrom s len
iterIO-0.2.2/Data/IterIO/Extra.hs:179:    genRecvFrom s len    = S.recvFrom s len
iterIO-0.2.2/Data/IterIO/Extra.hs:186:    genRecvFrom s len    = do (str, a) <- S.recvFrom s len
CMQ-0.0.12/src/System/CMQ.hs:236:        (msg, remoteSockAddr) <- recvFrom s 512 --influence on performance and best value still under investigation
udp-streaming-0.2.0.0/src/Streaming/UDP.hs:94:          msg <- liftIO $ recvFrom s sz
haskyapi-0.0.0.2/src/Web/Haskyapi.hs:80:  (str, _) <- recvFrom conn 2048
cacophony-0.10.1/tools/noise-repl/Socket.hs:30:              readSocket    = fst <$> recvFrom sock 65535
hcoap-0.1.2.1/src/Network/CoAP/Transport.hs:24:            , recvFrom = N.recvFrom sock 65535
distributed-process-simplelocalnet-0.2.4/src/Control/Distributed/Process/Backend/SimpleLocalnet/Internal/Multicast.hs:95:  (bytes, addr) <- NBS.recvFrom sock bufferSize
hosc-0.16/Sound/OSC/Transport/FD/UDP.hs:75:  (s,a) <- C.recvFrom fd 8192
moesocks-1.0.0.44/src/Network/MoeSocks/Handler.hs:69:                  recvFrom _localSocket _PacketLength
moesocks-1.0.0.44/src/Network/MoeSocks/Handler.hs:160:          (_msg, _sockAddr) <- recvFrom _remoteSocket _PacketLength
linear-socket-0.3.3.3/src/Network/Typed/Socket.hs:226:  (bs, sa) <- NSB.recvFrom s n
courier-0.1.1.5/src/Network/Transport/Sockets/UDP.hs:148:  (msg,_) <- NSB.recvFrom socket 512
futun-0.1.0.2/futun.hs:84:  (hello, addr) <- recvFrom sock 1024
xcp-0.1.0.1/Network/XcpEth.hs:217:  (res, addr) <- XcpEth . liftIO $ recvFrom sock (1024 * 10)
hosts-server-0.1.1/Main.hs:109:        (s, addr) <- recvFrom sock (bufSize conf)
network-enumerator-0.1.5/lib/Network/Socket/Enumerator.hs:54:	(bytes, addr) <- try (recvFrom sock intSize)
resolve-0.1.0.0/src/Resolve/DNS/UDP.hs:46:                                          (bs, sa) <- (recvFrom (socket c) maxLength)
wsjtx-udp-0.1.3.4/src/WSJTX/UDP/Server.hs:60:     (_,addr) <- recvFrom sock 1024
blubber-0.0.1/src-exec/blubber.hs:249:  (mesg, _) <- recvFrom s 1024
blubber-0.0.1/src-exec/blubber.hs:262:  (mesg, _) <- recvFrom s 1024
pocket-dns-0.1.1/Network/DNS/Pocket/Server.hs:124:      (bs, addr) <- recvFrom sock (bufSize def')
asn1-codec-0.2.0/src/TestNetwork.hs:56:        (bs,_) <- recvFrom sock 4096
shadowsocks-1.20180408/app/server.hs:46:        (encRequest, sourceAddr) <- recvFrom udpSocket 65535
shadowsocks-1.20180408/app/server.hs:61:            (packet', sockAddr) <- recvFrom remote 65535
krpc-0.6.1.0/src/Network/KRPC/Manager.hs:442:      handle exceptions $ BS.recvFrom sock (optMaxMsgSize options)
git-annex-7.20181121/Assistant/Threads/PairListener.hs:100:		(msg, _) <- B.recvFrom sock chunksz
Scurry-0.0.3/src/Scurry/Comm/SockSource.hs:23:    (msg,addr) <- recvFrom sock readLength
sws-0.4.3.0/Main.hs:82:    (bytes, addr) <- Net.recvFrom s 576
hdph-0.0.1/src/Control/Parallel/HdpH/Internal/Comm.hs:591:    (msg, _) <- recvFrom sock 1024

eborden · 2018-11-27T22:02:13Z

Here is a further breakdown of the function calls

fst

libjenkins-0.8.4/src/Jenkins/Discover.hs:70:readAnswer s = fst <$> ByteString.recvFrom s 4096
haskyapi-0.0.0.2/src/Web/Haskyapi.hs:80:  (str, _) <- recvFrom conn 2048
cacophony-0.10.1/tools/noise-repl/Socket.hs:30:              readSocket    = fst <$> recvFrom sock 65535
courier-0.1.1.5/src/Network/Transport/Sockets/UDP.hs:148:  (msg,_) <- NSB.recvFrom socket 512
blubber-0.0.1/src-exec/blubber.hs:249:  (mesg, _) <- recvFrom s 1024
blubber-0.0.1/src-exec/blubber.hs:262:  (mesg, _) <- recvFrom s 1024
asn1-codec-0.2.0/src/TestNetwork.hs:56:        (bs,_) <- recvFrom sock 4096
git-annex-7.20181121/Assistant/Threads/PairListener.hs:100:		(msg, _) <- B.recvFrom sock chunksz
hdph-0.0.1/src/Control/Parallel/HdpH/Internal/Comm.hs:591:    (msg, _) <- recvFrom sock 1024

snd

network-dns-1.1.0.1/examples/Resolver.hs:52:      (_, respBs) ← recvFrom sk 1024
wsjtx-udp-0.1.3.4/src/WSJTX/UDP/Server.hs:60:     (_,addr) <- recvFrom sock 1024

id

ntp-control-0.1/Network/NTP/Control.hs:24:  (b, sa) <- N.recvFrom s 1024
blubber-server-0.0.1/src-exec/blubber-server.hs:248:  (m, c) <- recvFrom sock 4096
zre-0.1.0.1/src/Network/ZRE/Beacon.hs:28:        (msg, addr) <- recvFrom sock 22
kademlia-1.1.0.0/src/Network/Kademlia/Networking.hs:89:        (received, addr) <- S.recvFrom (kSock kh) 1500
mvclient-0.4/src/Network/Metaverse/Circuit.hs:463:recvRaw sock = fmap (Just . first deserialize) (recvFrom sock 10000)
TORCS-0.1.0.2/TORCS/Connect/Runner.hs:66:  (msg,addr) <- recvFrom conn 1024
TORCS-0.1.0.2/TORCS/Connect/Runner.hs:100:  (msg,d) <- catch (recvFrom conn 1024) (\(e :: SomeException) -> return ("",SockAddrUnix "")) --if nothing to sense from, get default value
datadog-0.2.2.0/test/Test/Network/Datadog/StatsD.hs:36:      val <- timeout 10000000 $ recvFrom conn 2048
conduit-extra-1.3.0/Data/Conduit/Network/UDP.hs:34:        (bs, addr) <- lift $ liftIO $ recvFrom socket len
tremulous-query-1.0.7/Network/Tremulous/Polling.hs:66:            packet <- ioForce finished $ recvFrom sock mtu
secureUDP-0.1.1.3/SecureUDP.hs:163:    (bString,sAddr) <- B.recvFrom (socket chcfg) (maxPacketSize chcfg + 4)
iterIO-0.2.2/Data/IterIO/Extra.hs:172:    genRecvFrom s len    = do (str, a) <- S.recvFrom s len
iterIO-0.2.2/Data/IterIO/Extra.hs:179:    genRecvFrom s len    = S.recvFrom s len
iterIO-0.2.2/Data/IterIO/Extra.hs:186:    genRecvFrom s len    = do (str, a) <- S.recvFrom s len
CMQ-0.0.12/src/System/CMQ.hs:236:        (msg, remoteSockAddr) <- recvFrom s 512 --influence on performance and best value still under investigation
udp-streaming-0.2.0.0/src/Streaming/UDP.hs:94:          msg <- liftIO $ recvFrom s sz
hcoap-0.1.2.1/src/Network/CoAP/Transport.hs:24:            , recvFrom = N.recvFrom sock 65535
distributed-process-simplelocalnet-0.2.4/src/Control/Distributed/Process/Backend/SimpleLocalnet/Internal/Multicast.hs:95:  (bytes, addr) <- NBS.recvFrom sock bufferSize
hosc-0.16/Sound/OSC/Transport/FD/UDP.hs:75:  (s,a) <- C.recvFrom fd 8192
linear-socket-0.3.3.3/src/Network/Typed/Socket.hs:226:  (bs, sa) <- NSB.recvFrom s n
futun-0.1.0.2/futun.hs:84:  (hello, addr) <- recvFrom sock 1024
xcp-0.1.0.1/Network/XcpEth.hs:217:  (res, addr) <- XcpEth . liftIO $ recvFrom sock (1024 * 10)
hosts-server-0.1.1/Main.hs:109:        (s, addr) <- recvFrom sock (bufSize conf)
network-enumerator-0.1.5/lib/Network/Socket/Enumerator.hs:54:	(bytes, addr) <- try (recvFrom sock intSize)
resolve-0.1.0.0/src/Resolve/DNS/UDP.hs:46:                                          (bs, sa) <- (recvFrom (socket c) maxLength)
pocket-dns-0.1.1/Network/DNS/Pocket/Server.hs:124:      (bs, addr) <- recvFrom sock (bufSize def')
shadowsocks-1.20180408/app/server.hs:46:        (encRequest, sourceAddr) <- recvFrom udpSocket 65535
shadowsocks-1.20180408/app/server.hs:61:            (packet', sockAddr) <- recvFrom remote 65535
krpc-0.6.1.0/src/Network/KRPC/Manager.hs:442:      handle exceptions $ BS.recvFrom sock (optMaxMsgSize options)
Scurry-0.0.3/src/Scurry/Comm/SockSource.hs:23:    (msg,addr) <- recvFrom sock readLength
sws-0.4.3.0/Main.hs:82:    (bytes, addr) <- Net.recvFrom s 576

void

moesocks-1.0.0.44/src/Network/MoeSocks/Handler.hs:160:          (_msg, _sockAddr) <- recvFrom _remoteSocket _PacketLength

Mistuke · 2018-11-27T22:22:04Z

hmm perhaps split the functions up, make a new one that doesn't call getPeerName and only returns IO Int and one that has the current signature. and then use a ghc rewrite rule to rewrite the fst cases into the new more efficient call?

That still leaves the exception being removed, but that can be communicated via a changelog?

eborden · 2018-11-27T22:26:23Z

I like that plan. That way we are changing a type along with the behavior change.

--recvBufFrom :: SocketAddress sa => Socket -> Ptr a -> Int -> IO (Int, sa)
++recvBufFrom :: Socket -> Ptr a -> Int -> IO Int

++recvBufFromWithAddress :: SocketAddress sa => Socket -> Ptr a -> Int -> IO (Int, sa)

Mistuke · 2018-11-27T22:30:44Z

Yeah, that should make things a bit more obvious :)

eborden · 2018-11-27T22:35:50Z

Oh you know what, that won't work because of withNewSocketAddress. Since we've moved to SocketAddress sa this introduces ambiguity. So we'd either need a Proxy sa or AllowAmbiguousTypes and TypeApplications. Both of these don't sound like great options for network.

Mistuke · 2018-11-27T22:46:02Z

Ambiguity between which functions? don't all three of those have different types?

eborden · 2018-11-27T23:37:56Z

In this definition we get ambiguity on withNewSocketAddress

recvBufFrom :: Socket -> Ptr a -> Int -> IO Int
recvBufFrom s ptr nbytes
    | nbytes <= 0 = ioError (mkInvalidRecvArgError "Network.Socket.recvBufFrom")
    | otherwise = withNewSocketAddress $ \ptr_sa sz -> alloca $ \ptr_len -> do
        fd <- fdSocket s
        poke ptr_len (fromIntegral sz)
        let cnbytes = fromIntegral nbytes
            flags = 0
        len <- throwSocketErrorWaitRead s "Network.Socket.recvBufFrom" $
                 c_recvfrom fd ptr cnbytes flags ptr_sa ptr_len
        return $ fromIntegral len

kazu-yamamoto · 2018-11-28T04:01:20Z

Before mentioning my opinion, I would like to ask one question about recvBufFrom:

        sockaddr <- peekSocketAddress ptr_sa
            `E.catch` \(E.SomeException _) -> getPeerName s

When does peekSocketAddress throw an exception?

eborden · 2018-11-29T03:57:58Z

I imagine there are others, but a primary error is defined in Network.Socket.Types.hsc

    _ -> ioError $ userError $
      "Network.Socket.Types.peekSockAddr: address family '" ++
      show family ++ "' not supported."

Mistuke · 2018-11-29T20:10:04Z

In this definition we get ambiguity on withNewSocketAddress

recvBufFrom :: Socket -> Ptr a -> Int -> IO Int
recvBufFrom s ptr nbytes
    | nbytes <= 0 = ioError (mkInvalidRecvArgError "Network.Socket.recvBufFrom")
    | otherwise = withNewSocketAddress $ \ptr_sa sz -> alloca $ \ptr_len -> do
        fd <- fdSocket s
        poke ptr_len (fromIntegral sz)
        let cnbytes = fromIntegral nbytes
            flags = 0
        len <- throwSocketErrorWaitRead s "Network.Socket.recvBufFrom" $
                 c_recvfrom fd ptr cnbytes flags ptr_sa ptr_len
        return $ fromIntegral len

Ahh right, hmm.. that's a bit unfortunate..

Mistuke · 2018-12-02T22:37:28Z

I guess the big concern here is backwards compat. Perhaps we should just name the new calls something else and just deprecate the old ones? remove them in the next major version?

kazu-yamamoto · 2018-12-03T00:44:57Z

I would also like to clarify the semantics of the old function: the combination of peekSocketAddress and getPeerName.

kazu-yamamoto · 2018-12-20T07:29:53Z

To maintain backward compatiblity on function name level and to infer sockaddr type, I would propose merge this PR as is.

kazu-yamamoto · 2018-12-21T02:19:02Z

@eborden @Mistuke Ping

We should update the docs of related functions.

eborden · 2018-12-21T02:22:54Z

I'm likely not going to get to this till the new year, but if anyone has the motivation, please commit and merge.

kazu-yamamoto · 2019-01-07T03:38:37Z

Merged with doc update.

eborden added the RFC label Nov 26, 2018

eborden self-assigned this Nov 26, 2018

eborden requested review from kazu-yamamoto and Mistuke November 26, 2018 16:53

eborden force-pushed the eborden/remove-eof-errors branch from 7968232 to d28580e Compare November 26, 2018 20:49

kazu-yamamoto added a commit to kazu-yamamoto/network that referenced this pull request Jan 7, 2019

Merge PR haskell#360.

c503a1b

kazu-yamamoto closed this Jan 7, 2019

kazu-yamamoto deleted the eborden/remove-eof-errors branch January 7, 2019 03:39

eborden mentioned this pull request Jan 19, 2019

Release 3.0.0.0 #357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove EOF errors from receive functions #360

Remove EOF errors from receive functions #360

eborden commented Nov 26, 2018

Mistuke commented Nov 26, 2018

eborden commented Nov 26, 2018

kazu-yamamoto commented Nov 27, 2018

eborden commented Nov 27, 2018 •

edited

Loading

eborden commented Nov 27, 2018

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

kazu-yamamoto commented Nov 28, 2018

eborden commented Nov 29, 2018 •

edited

Loading

Mistuke commented Nov 29, 2018

Mistuke commented Dec 2, 2018

kazu-yamamoto commented Dec 3, 2018

kazu-yamamoto commented Dec 20, 2018

kazu-yamamoto commented Dec 21, 2018

eborden commented Dec 21, 2018

kazu-yamamoto commented Jan 7, 2019

Remove EOF errors from receive functions #360

Remove EOF errors from receive functions #360

Conversation

eborden commented Nov 26, 2018

Mistuke commented Nov 26, 2018

eborden commented Nov 26, 2018

kazu-yamamoto commented Nov 27, 2018

eborden commented Nov 27, 2018 • edited Loading

recvBufFrom

recvFrom

eborden commented Nov 27, 2018

fst

snd

id

void

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

Mistuke commented Nov 27, 2018

eborden commented Nov 27, 2018

kazu-yamamoto commented Nov 28, 2018

eborden commented Nov 29, 2018 • edited Loading

Mistuke commented Nov 29, 2018

Mistuke commented Dec 2, 2018

kazu-yamamoto commented Dec 3, 2018

kazu-yamamoto commented Dec 20, 2018

kazu-yamamoto commented Dec 21, 2018

eborden commented Dec 21, 2018

kazu-yamamoto commented Jan 7, 2019

eborden commented Nov 27, 2018 •

edited

Loading

`recvBufFrom`

`recvFrom`

eborden commented Nov 29, 2018 •

edited

Loading