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

Add support for UDP #129

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Add support for UDP #129

merged 8 commits into from
Nov 15, 2024

Conversation

lhoward
Copy link
Contributor

@lhoward lhoward commented Nov 11, 2024

No description provided.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 37.76824% with 290 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (3de9f8a) to head (7731068).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
FlyingSocks/Sources/Socket.swift 30.67% 217 Missing ⚠️
FlyingSocks/Sources/AsyncSocket.swift 27.27% 64 Missing ⚠️
FlyingSocks/Sources/Socket+Darwin.swift 50.00% 6 Missing ⚠️
FlyingSocks/Sources/SocketAddress.swift 93.75% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3de9f8a) and HEAD (7731068). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (3de9f8a) HEAD (7731068)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
- Coverage   96.66%   89.62%   -7.04%     
==========================================
  Files          60       60              
  Lines        3504     3932     +428     
==========================================
+ Hits         3387     3524     +137     
- Misses        117      408     +291     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lhoward
Copy link
Contributor Author

lhoward commented Nov 11, 2024

You mean I have to write tests? ;)

@swhitty
Copy link
Owner

swhitty commented Nov 11, 2024

This is really great thank you.

I think the changes to Socket* files are 👌🏼

I don't think we necessarily need AsyncMessageSocket as we should be able to achieve the same with adding methods to AsyncSocket. I've performed a few tests and errors should thrown (on Darwin at least) if users attempt to call the wrong methods when using SOCK_DGRAM.

I've have built a little echo server using your code with a couple of small tweaks:

echo

From here it would be easy to add a AsyncSequence of (Socket.Address, [UInt8]).
(I used Socket.Address because its easier to print out to console but could also be sockaddr_storage)

No need to worry about tests until we settle on the right API

@lhoward
Copy link
Contributor Author

lhoward commented Nov 11, 2024

Great, have applied your diff locally, will push an update shortly.

@lhoward
Copy link
Contributor Author

lhoward commented Nov 11, 2024

I do need sockaddr_storage to avoid redundant string conversions in our application.

@lhoward lhoward force-pushed the udp branch 6 times, most recently from fbe7c44 to 9c7be02 Compare November 12, 2024 00:36
Copy link
Owner

@swhitty swhitty left a comment

Choose a reason for hiding this comment

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

this is wonderful, thank you

@swhitty
Copy link
Owner

swhitty commented Nov 12, 2024

Because Swift Testing runs tests in parallel you may need to bind to port 0 letting the OS provide a port number then retrieve it and send to it

let address: any SocketAddress = .loopback(port: 0)
let socket = try Socket(domain: Int32(type(of: address).family), type: Int32(SOCK_DGRAM))
try socket.bind(to: address)
let listenAddr = try socket.sockname() // gets port

@lhoward
Copy link
Contributor Author

lhoward commented Nov 12, 2024

Because Swift Testing runs tests in parallel you may need to bind to port 0 letting the OS provide a port number then retrieve it and send to it

let address: any SocketAddress = .loopback(port: 0)

let socket = try Socket(domain: Int32(type(of: address).family), type: Int32(SOCK_DGRAM))

try socket.bind(to: address)

let listenAddr = try socket.sockname() // gets port

Great, I ran out of time today but I'll take a look tomorrow!

@lhoward
Copy link
Contributor Author

lhoward commented Nov 12, 2024

BTW, UDP sockets can be connected and there can be some benefits to use this on the client side (it can save a route lookup as it can be cached in the "connection"). I'll add support for that too.

Edit: actually we can probably just use read and write, they should be identical to recv and send with no flags (shall triple check).

@lhoward
Copy link
Contributor Author

lhoward commented Nov 12, 2024

We might want to support multihomed servers without needing to bind separate sockets to each interface.

@lhoward
Copy link
Contributor Author

lhoward commented Nov 12, 2024

We might want to support multihomed servers without needing to bind separate sockets to each interface.

So for this I reckon maybe we want to make the message a structure rather than a tuple (as I had it originally, although I'm not wedded to this), and then add the socket address on which the packet was received and the interface index. These should be usable on both send and receive and be optional (or possibly omitted entirely from the structure if the underlying platform doesn't support).

Something like:

struct /*AsyncSocket.*/Message: Sendable {
    let data: [UInt8]
    let peerAddress: sockaddr_storage
    let localAddress: sockaddr_storage?
    let interfaceIndex: Int32?
}

Dealing with struct msghdr in Swift is a pain but I think I have some code somewhere that I can cut and paste (probably in IORingSwift).

@swhitty
Copy link
Owner

swhitty commented Nov 13, 2024

that's fine, its an additional feature so can skip a platform without breaking anything

@lhoward
Copy link
Contributor Author

lhoward commented Nov 13, 2024

~~For Windows, AsyncSocketMessageSequence returns a (sockaddr_storage, [UInt8]). It may be more sensible to just remove this on Windows entirely so that we don't have a breaking API change when it's implemented properly.

On the other hand, use cases for a multihomed UDP server on Windows... I mean, it seems unlikely to me but, I'm sure someone can think of one.~~

Scratch that, Windows API is now consistent, it just doesn't return the local interface index and address.

public struct AsyncSocketMessageSequence: AsyncSequence, AsyncIteratorProtocol, Sendable {
    public static let DefaultMaxMessageLength: Int = 1500

    // Windows has a different recvmsg() API signature which is presently unsupported
    public typealias Element = AsyncSocket.Message

    private let socket: AsyncSocket
    private let maxMessageLength: Int

    public func makeAsyncIterator() -> AsyncSocketMessageSequence { self }

    init(socket: AsyncSocket, maxMessageLength: Int = Self.DefaultMaxMessageLength) {
        self.socket = socket
        self.maxMessageLength = maxMessageLength
    }

    public mutating func next() async throws -> Element? {
#if !canImport(WinSDK)
        try await socket.receive(atMost: maxMessageLength)
#else
        let peerAddress: sockaddr_storage
        let bytes: [UInt8]

        (peerAddress, bytes) = try await socket.receive(atMost: maxMessageLength)
        return AsyncSocket.Message(peerAddress: peerAddress, bytes: bytes)
#endif
    }
}

@lhoward lhoward force-pushed the udp branch 10 times, most recently from b384839 to 6fb4b23 Compare November 14, 2024 00:33
Insulate callers from libc types by adding a `SocketType` enumerated type, that
can either be `stream` or `datagram`.
@lhoward lhoward force-pushed the udp branch 2 times, most recently from 187104e to 9ce84ec Compare November 14, 2024 05:51
@lhoward
Copy link
Contributor Author

lhoward commented Nov 14, 2024

I've made a couple of other changes which you may/may not wish to take. I was a bit annoyed by needing to use sockaddr_storage so I've had the new APIs return any SocketAddress (which is a sockaddr_storage, which now conforms to SocketAddress). I will add some more tests of this too.

And also there's an AnySocketAddress type erased wrapper which is useful if you need to pass it any to something that takes some SocketAddress.

@lhoward lhoward force-pushed the udp branch 3 times, most recently from 27c008b to 1705f87 Compare November 14, 2024 08:50
Add support for datagram sockets, wrapping sendto() and recvfrom(). Note:
send() and recv() are not supported, so a destination address must be supplied;
however write() and read() can be used with datagram sockets.

Fixes: swhitty#128
Multihomed servers (i.e. those with multiple network interfaces) need to take
care to send UDP responses from the interface on which they were received,
otherwise the client may never receive the packet.

Historically this was done by binding separate sockets to each interface,
however this does not adapt well to dynamic interface changes (without extra
code to monitor for this, which is impossible to do in a portable manner).

Modern operating systems provide `IP_PKTINFO` and `IPV6_PKTINFO` which allow
the local interface index and address to be set and reported on unbound
sockets. This commit adds support for this.

Note: currently unavailable on Windows.
Copy link
Owner

@swhitty swhitty left a comment

Choose a reason for hiding this comment

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

Great work

@swhitty swhitty merged commit 150911e into swhitty:main Nov 15, 2024
7 of 9 checks passed
@lhoward
Copy link
Contributor Author

lhoward commented Nov 16, 2024

Thanks Simon, really helps out my project too having this merged!

@swhitty
Copy link
Owner

swhitty commented Nov 21, 2024

I was adding some more unit tests and discovered an issue with IP6 on both Darwin and Linux:

try Socket(domain: AF_INET6, type: .datagram)

This always throws an error:
Darin: SocketError.failed(type: "SetOption", errno: 13, message: "Permission denied")
Linux: SocketError.failed(type: "SetPktInfoOption", errno: 22, message: "Invalid argument")

While I have not fixed the issue, I have a PR #131 that I would appreciate your input on, because it replaces the private func setPktInfo() API you added here allowing users to sidestep the issue.

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.

2 participants