Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting sendMsg and recvMsg. #433

Merged
merged 48 commits into from
Apr 8, 2020
Merged

Conversation

kazu-yamamoto
Copy link
Collaborator

Note that the quality of this code is good enough, I believe. But I don't expect that this PR will be merged quickly. I would like to merge this PR after discussion.

Cc: @snoyberg, @farnoy, @ocheron, @lukehoersten, @lpeterse, @vdukhovni

Motivation

  • We would like to use MSG_PEEK for TCP sockets to peek the received data without removing the input queue.
  • We would like to receive or specify, if possible, the control message such as IP_TTL, IP_TOS for ECN and IP_PKTINFO to specify the outgoing network interface for UDP packets.

So, I would like to discuss how to support sendmsg() and recvmsg(). Note that they can be used for TCP in addition to UDP.

Design

The current wrapper APIs for sendmsg() and recvmsg() are as follows:

sendMsg :: Socket       -- ^ Socket
        -> SockAddr     -- ^ Destination address
        -> [ByteString] -- ^ Data to be sent
        -> [Cmsg]       -- ^ Control messages
        -> [MsgFlag]    -- ^ Message flags
        -> IO Int       -- ^ The length actually sent

recvMsg :: Socket    -- ^ Socket
        -> [Int]     -- ^ A list of length of data to be received
                     --   If the total length is not large enough,
                     --   'MSG_TRUNC' is returned
        -> Int       -- ^ The buffer size for control messages.
                     --   If the length is not large enough,
                     --   'MSG_CTRUNC' is returned
        -> [MsgFlag] -- ^ Message flags
        -> IO (SockAddr, [ByteString], [Cmsg], [MsgFlag]) -- ^ Source address, received data, control messages and message flags
  • MsgFlag includes MSG_PEEK
  • Cmsg is a low level format for sendMsg and recvMsg. Their values can be converted into user-friendly type values such as IPv4TTL and IPv4PktInfo via Auxiliary class.

I believe we don't have to extend, for instance, recv to support MsgFlag since recvMsg is a superset.

Please read test cases to know how to use these APIs more concretely.

Discussion

  • @eborden Should sendMsg/recvMsg be supported in network. Or another package?
  • @Mistuke How can we support Windows? Does Windows provide similar APIs?

Note

  • IPv4TTL/IPv6HopLimit and IPv4TOS/IPv6TClass are reading-purpose only.
  • IPv4PktInfo and IPv6PktInfo for sendMsg are not tested yet.
  • This is relating to Support passing flags to c_recv #243.

@maoe
Copy link
Member

maoe commented Dec 4, 2019

Coincidentally, I wrote similar code for recvmmsg recently to use hardware timestamping. I'll review this PR when I have time.

@kazu-yamamoto kazu-yamamoto changed the title Supportint sendMsg and recvMsg. Supporting sendMsg and recvMsg. Dec 4, 2019
@eborden
Copy link
Collaborator

eborden commented Dec 5, 2019

Should sendMsg/recvMsg be supported in network. Or another package?

If we can support it cross platform then I'm in favor of inclusion. @Mistuke's answer about Windows will be key in that decision.

@kazu-yamamoto
Copy link
Collaborator Author

@eborden I agree.

@kazu-yamamoto
Copy link
Collaborator Author

@Mistuke I would like to forward this issue. Would you give us your advice? Are you in mountains again? :-)

Copy link
Member

@maoe maoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the API could have been lower-level to reduce some overhead and to make C-interop easier.

For example in the proposed API recvMsg returns lists of ByteStrings and Cmsgs. This can be a costly process to run in low-latency networking code because every payload and ancillary data needs to be copied over to the Haskell heap. Instead it would be nice if we had something analoguous to recvBuf.

Network/Socket/ByteString.hs Outdated Show resolved Hide resolved
Network/Socket/ByteString/Cmsg.hsc Outdated Show resolved Hide resolved
Network/Socket/ByteString/Cmsg.hsc Outdated Show resolved Hide resolved
Network/Socket/ByteString/Cmsg.hsc Outdated Show resolved Hide resolved
Network/Socket/ByteString/MsgHdr.hsc Show resolved Hide resolved
Network/Socket/ByteString/Auxiliary.hsc Outdated Show resolved Hide resolved
Network/Socket/ByteString/IO.hsc Outdated Show resolved Hide resolved
@Mistuke
Copy link
Collaborator

Mistuke commented Dec 22, 2019

@Mistuke I would like to forward this issue. Would you give us your advice? Are you in mountains again? :-)

Sorry, was very very busy with work. I will go over this today.

@kazu-yamamoto
Copy link
Collaborator Author

OK. I will try to implement this feature in network and resolve all issues pointed by the reviewers.

@kazu-yamamoto
Copy link
Collaborator Author

kazu-yamamoto commented Jan 10, 2020

Additional plan:

  • Supporting Windows
  • Testing ancillary data especially for IPv4PktInfo and IPv6PktInfo
  • Generalizing sendBufMsg and recvBufMsg with SocketAddress
  • Replacing C implementation of sendFd and recvFd with this API
  • Implementing extendable socket option functions
  • Checking CChar vs CInt

@kazu-yamamoto
Copy link
Collaborator Author

@maoe @vdukhovni Would you review this again?

@Mistuke Now it's your turn. If time allows, please implement Windows part.

@kazu-yamamoto
Copy link
Collaborator Author

It appeared that IP_PKTINFO of macOS is buggy. If it used with sendMsg with either non-null ipi_ifindex or non-null ipi_spec_dst, the local address of the socket is overridden with 0.0.0.0.

@Mistuke
Copy link
Collaborator

Mistuke commented Jan 19, 2020

@kazu-yamamoto Sorry missed your message, I will get started on this next Sunday (suspect to be able to get it done same week). Currently untangling some GHC release issues :(

@kazu-yamamoto
Copy link
Collaborator Author

@Mistuke No rush. But please keep in mind this issue. :-)

I'm currently testing this PR with my QUIC implementation.

@kazu-yamamoto
Copy link
Collaborator Author

Rebased onto master.

Network/Socket/Flag.hsc Outdated Show resolved Hide resolved
@Mistuke
Copy link
Collaborator

Mistuke commented Feb 18, 2020

I haven't forgotten about this! I'm mapping the changes I need to do out and will refactor it and hopefully have something soon!

@Mistuke
Copy link
Collaborator

Mistuke commented Feb 23, 2020

WIP PR for Windows support at at kazu-yamamoto#2, should be able to finish it next weekend.

@kazu-yamamoto
Copy link
Collaborator Author

@Mistuke Gentle ping

@Mistuke
Copy link
Collaborator

Mistuke commented Mar 16, 2020

looks like RecvMsg needs some work, I'll try to do that today after work so this is done :)

@Mistuke
Copy link
Collaborator

Mistuke commented Mar 22, 2020

Haven't forgotten, still debugging WSARecvMsg the failure is a bit generic..

Copy link

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some mostly documentation-related comments. The code looks fine to me at first glance.

Network/Socket/Buffer.hsc Outdated Show resolved Hide resolved
Network/Socket/Buffer.hsc Outdated Show resolved Hide resolved

----------------------------------------------------------------

-- | Looking up control message. The following shows an example usage:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of "Looking up control message", better might be:

Locate a control message of the given type in a list of control messages

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is this function really necessary? It is the same as Data.List.find . (. cmsgId) . (==).
I am not sure it needs to be provided here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the doc and the definition.

But is this function really necessary? It is the same as Data.List.find . (. cmsgId) . (==).

Not sure. But I think it's useful as suggested in the usage.

| cmsgId cmsg == cid = Just cmsg
| otherwise = lookupCmsg cid cmsgs

-- | Filtering control message.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too might be better rephrased, but again, why not just drop it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you provided the better doc.

foreign import ccall SAFE_ON_WIN "recvFd" c_recvFd :: CInt -> IO CInt
recvFd s = allocaBytes dummyBufSize $ \buf -> do
(NullSockAddr, _, cmsgs, _) <- recvBufMsg s [(buf,dummyBufSize)] 32 mempty
case (lookupCmsg CmsgIdFd cmsgs >>= decodeCmsg) :: Maybe Fd of

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here find ((== CmsgIdFd) . cmsgId) cmsgs could replace lookupCmsg.

appveyor.yml Outdated
- GHCVER: 8.0.2
- GHCVER: 8.2.2
- GHCVER: 8.4.4
- GHCVER: 8.6.3
- GHCVER: 8.8.1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By now perhaps 8.8.2 or 8.8.3, whichever is the latest available on Appveyor?

@Mistuke
Copy link
Collaborator

Mistuke commented Apr 2, 2020

Almost done, it's a bit slow going debugging the API. I have a working C example now fixing the Haskell side. hopefully done this weekend.

@Mistuke
Copy link
Collaborator

Mistuke commented Apr 5, 2020

@kazu-yamamoto That should do it. Sorry for the delay. I've been a bit over stretched lately.

The only thing I was wondering about is if I can simplify the logic in recvBufMsg, particularly this part

#if !defined(mingw32_HOST_OS)
              , msgCtrl    = castPtr ctrlPtr
#else
              , msgCtrl    = if clen == 0 then nullPtr else castPtr ctrlPtr
#endif
              , msgCtrlLen = fromIntegral clen
#if !defined(mingw32_HOST_OS)
              , msgFlags   = 0
#else
              , msgFlags   = fromIntegral $ fromMsgFlag flags
#endif

Would it present a problem for Linux if when clen == 0 we always pass nullPtr and if we always encode the flag in msgFlags ?

@vdukhovni
Copy link

Would it present a problem for Linux if when clen == 0 we always pass nullPtr and if we always encode the flag in msgFlags ?

I've not checked Linux, but it would be OK to always encode the flags on FreeBSD where I checked the kernel source, and the recvmsg() flags argument is copied to msghdr.msg_flags when collecting the system call arguments, and later replaced with the output flags.

However, setting msgCtrl to NULL when clen == 0 changes semantics. With a non-null msgCtrl, but clen == 0 you get MSG_CTRUNC in the returned flags when there were control messages that were discarded. With msgCtrl == NULL the flag is not set. (Again FreeBSD).

I don't know whether anyone depends on this behaviour, but it is perhaps best to not make that change on Unix-like systems.

@kazu-yamamoto kazu-yamamoto merged commit 54b872f into haskell:master Apr 8, 2020
@kazu-yamamoto kazu-yamamoto deleted the msg branch April 8, 2020 05:43
@kazu-yamamoto
Copy link
Collaborator Author

This PR has been merged. Thank you for your contribution! Let's discuss left issues in another PR.

@kazu-yamamoto kazu-yamamoto mentioned this pull request May 14, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants