Skip to content

[WIP] UDP Networking #1331

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

Closed
wants to merge 21 commits into from
Closed

Conversation

kristate
Copy link
Contributor

@kristate kristate commented Aug 4, 2018

This is WIP based off of the async-fs branch.

Adding memo from IRC

<andrewrk> kristate, I think it's good you followed the API pattern from std.event.fs.Watch for UDP.
<andrewrk> I think it's going to be nice, having the packets come from a Channel 
<andrewrk> I think we should also update the tcp code that is currently there to have this API 
<kristate> andrewrk: I agree 

Ref #1271

const mem = std.mem;
const net = std.net;

pub fn UDP() type {
Copy link
Member

Choose a reason for hiding this comment

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

if it doesn't take any comptime args, then you can just do pub const UDP = struct { ...


while (true) {
const rc = os.darwin.read(fd, &event_buf, event_buf.len);
const errno = os.darwin.getErrno(rc); //maybe this shouldn't be called so much?
Copy link
Member

Choose a reason for hiding this comment

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

can you explain this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could be wrong, but most net libraries (including the ones I have wrote in the past) will check to see if rc is less than 0 and then call getErrno.

Perhaps we can change const errno = os.darwin.getErrno(rc);
to something like const errno = if (rc < 0) os.darwin.getErrno(rc) else 0;

Copy link
Member

@andrewrk andrewrk Aug 4, 2018

Choose a reason for hiding this comment

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

Ah. In std.os.darwin, we hide errno from the user and make it a normal part of the return code, just like it is if you do the syscall directly. std.os.darwin is intended to be an API that is agnostic of whether it goes through libSystem or the kernel directly, just like std.os.linux. (Perhaps we will create static darwin binaries someday rather than linking libSystem.)

This function is not the errno function. Here's the definition:

/// Get the errno from a syscall return value, or 0 for no error.
pub fn getErrno(r: usize) usize {
    const signed_r = @bitCast(isize, r);
    return if (signed_r > -4096 and signed_r < 0) @intCast(usize, -signed_r) else 0;
}

std.os.linux has the same definition.

Here is errnoWrap:

/// Takes the return value from a syscall and formats it back in the way
/// that the kernel represents it to libc. Errno was a mistake, let's make
/// it go away forever.
fn errnoWrap(value: isize) usize {
    return @bitCast(usize, if (value == -1) -isize(c._errno().*) else value);
}

Note that with optimizations on, these number fiddlings go away and it compiles down to checking -1 and calling _errno(), the same as if we didn't do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good stuff!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit weird. Or maybe the errnoWrap function. Because read returns the number of bytes read iirc or an error code when it's <0. The errnoWrap makes it always return something positive and the only way to figure out if it's an error is by calling getErrno?

Maybe these errors should actually be errors and not some vague return value magic?

(assuming this works the same as Linux, otherwise discard my comment. It's still weird though imo.)

@kristate
Copy link
Contributor Author

kristate commented Aug 5, 2018

memo: Since this is based on #1333, Postponed until #1333 clears.

@andrewrk andrewrk added the work in progress This pull request is not ready for review yet. label Aug 20, 2018
var event_buf: [1500]u8 = undefined;

while (true) {
const rc = os.darwin.read(fd, &event_buf, event_buf.len);
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to use recvmsg() here, and the buf above should support jumbo frames (9000 mtu). On Linux you should use recvmmsg(). I don't think a buf of exactly 9000 is correct with recvmsg() because the ip header stuff is not included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. This is what I currently have locally:

+            const rc = os.posix.recvfrom( ev_handle
                                        , &event_buf
                                        , event_buf.len
                                        , 0
                                        , &recv_addr.sa.u.sa
                                        , &recv_addr.sa.len
                                        );

            const errno = os.posix.getErrno(rc);
            switch (errno) {
                0 => {
                    std.debug.warn("PACKET: {}\n", &event_buf);
                    await (async channel.put(Self.Event{
                        .id = UDPEventId.Packet,
                        .address = recv_addr,
                        //.packet = &event_buf,
                    }) catch unreachable);
                },
                os.posix.EINTR => continue,
                os.posix.EINVAL => unreachable,
                os.posix.EFAULT => unreachable,
                // TODO this needs to try to re-up the Handler for the socket
                // iPhone is known for doing throwing this when a handoff occurs
                os.posix.ENOTCONN => unreachable,
                os.posix.EAGAIN => {
                    _ = (await (async loop.waitEvHandle(
                        ev_handle,
                        // https://github.com/ziglang/zig/issues/485
                        event.Loop.EventFlags { .READ = true
                                              , .WRITE = false
                                              , .EXCEPT = false
                                              , .reserved = undefined },
                    ) catch unreachable)) catch |err| {
                        const transformed_err = switch (err) {
                            error.AccessDenied,
                            error.EventNotFound,
                            error.ProcessNotFound,
                            error.OutOfMemory => unreachable,
                            error.SystemResources => UDPEventError.SystemResources,
                        };
                        await (async channel.put(transformed_err) catch unreachable);
                        return;
                    };
                },
                else => unreachable,

Copy link
Contributor

@shawnl shawnl 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 the wrong approach for TCP, with all its re-ordering of packet logic, esp. as it won't work with Linux's new zero-copy receive API. https://lwn.net/Articles/754681/

@kristate
Copy link
Contributor Author

@shawnl This branch is older than what I have locally -- I was waiting for #1294 to clear.
I will post what I have shortly. Thanks for your input.

@shawnl
Copy link
Contributor

shawnl commented Aug 24, 2018

That was a bit harsh. A channel API could still work for TCP, it just wouldn't be packet-incremented, and should use a larger buffer.

@shawnl
Copy link
Contributor

shawnl commented Aug 25, 2018

If you use this approach I don't think you need #1294. UDP doesn't have ordering so write from whatever thread produces the data. We can even compile the channel out on Linux with the SO_REUSEPORT socket option.

.family = posix.AF_INET,
.port = std.mem.endianSwapIfLe(u16, port),
.addr = ip4,
.addr = std.mem.endianSwapIfLe(u32, ip4),
Copy link
Contributor

Choose a reason for hiding this comment

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

net.parseIp4() assumes ip addresses are big-endian.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shawnl Thanks. I redid net.parseIp4 in the update. I am cleaning it up before I post the next WIP.

@kristate
Copy link
Contributor Author

@shawnl #1294 had lots of additions and has since been committed to master -- as mentioned two days ago I have an update that I will post next week. Please say hello to my brother at lunch!

@andrewrk andrewrk added this to the 0.4.0 milestone Aug 25, 2018
@andrewrk
Copy link
Member

Please re-open when it's ready for review.

@andrewrk andrewrk closed this Sep 15, 2018
@andrewrk andrewrk modified the milestones: 0.4.0, 0.3.0 Sep 28, 2018
@nxrighthere
Copy link

What's the current status of this PR? It seems abandoned.

@Saiv46 Saiv46 mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress This pull request is not ready for review yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants