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

Draft UDP socket support, inspired by Node's dgram module #14

Closed
wants to merge 1 commit into from

Conversation

lichtblau
Copy link
Contributor

Mostly functional UDP socket support.

Bit rough around the edges, but tests pass on Linux, and the new tests also seem to work on Darwin ("swift test" only, after deleting existing tests which don't work).

TODO: Updating the Xcode project for new sources and tests.

Copy link
Member

@helje5 helje5 left a comment

Choose a reason for hiding this comment

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

Ah, thanks. Looks mostly OK, though it is duplicating quite a lot of code. Maybe we can share more code with the socket stream. I'll review and merge later.

I'm also wondering whether dgram's should be streams (streams of byte packets), so that you can use pipes to parse them. Like

dgram.createSocket() | ntpParser | ... deal with packet objects instead of bytes ...

@helje5
Copy link
Member

helje5 commented Mar 20, 2017

I pushed that into the feature/dgram branch.

@helje5 helje5 closed this Mar 20, 2017
@helje5
Copy link
Member

helje5 commented Mar 20, 2017

I did a few changes, please review. I think I could merge that into develop that way though it would be kinda cool to have this as a proper stream first.

@lichtblau
Copy link
Contributor Author

Thanks!

Node doesn't actually implement the dgram sockets as streams, does it? It sounds like a nice feature. There could be two different use cases:

One is a byte stream. This might make sense (only) in the specialized case where we would support the connect syscall, which instructs a UDP socket to only ever send data to a specific peer, and only accept that from that peer. So in practise it's for UDP clients, but not servers. Using connect is possibly a bit exotic, but BSD sockets offer it.

The other is to keep supporting the generic case. The objects being streamed would be "Datagram" pairs of bytes and peer -- in both directions though, both when sending and receiving.

Personally this is not a priority for me. I'd be happy to do any other cleanups required for merging though, if you can propose something.

@helje5
Copy link
Member

helje5 commented Mar 22, 2017

Node doesn't actually implement the dgram sockets as streams, does it?

Node does not (really) have streams of arbitrary types the way Noze.io has it. Technically it does have object streams, but only with one object at the time, making it kinda useless for real world usage (and AFAIK people hardly use byte streams in Node, let alone object streams ;-).

In Noze.io, if something emits same-or-similar typed objects, it almost always can and should be done as a stream as that offers so much extra l1337 (transforms, pipes, back-pressure, etc).

I can't follow you on the byte stream thing. UDP doesn't guarantee delivery order. An UDP stream should just deliver packets it receives. Packet ordering would be a thing a stream on top could do.

As mentioned I think it would be a Duplex<Datagram, [UInt8]>. The readable end would provide Datagram structs (sender-address + payload) while you would write payloads ([UInt8]) into it (the address is implicit in this case, right? Or not? Hm.)

Personally this is not a priority for me.

Never mind, this is a nice to have, not a must have. Probably going to merge into develop and possibly master tomorrow.

@helje5
Copy link
Member

helje5 commented Mar 24, 2017

Merged into develop. Will do master when Travis is through.

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.

None yet

2 participants