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 multicast #7423

Merged
merged 1 commit into from Jun 4, 2019

Conversation

@stakach
Copy link
Contributor

commented Feb 13, 2019

currently I've only added the LibC flags for Darwin but thought I'd get some feedback before proceeding further.

fixes: #3825

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

ping @ysbaddaden what do you think?

@RX14 the Windows implementation of sockets, the stuff in netinet/in.cr for BSD and linux, is almost identical to the BSD implementation, two minor differences. Was thinking of adding a winsock2.cr file with that in it, but not sure if it will compile / don't want to break anything you are working on.
Definitions for things like SaFamilyT are probably missing on Windows.
Did you want me to include it?

src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

Thanks @Sija I've fixed those issues with the comments

@Sija

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

@stakach Great! I'd also put all integer values inside backticks, since they ought to be treated literally.

@stakach stakach changed the title WIP: add support for UDP multicast add support for UDP multicast Feb 16, 2019

@ysbaddaden
Copy link
Member

left a comment

Please use actual POSIX namings where appropriate, however how ugly they can be. Using custom namings will work but searching for documentation, actual C definitions or porting to another POSIX platform is made unnecessary complex and confusing. It took me while to find what were the actual definitions for the structs, which should be translated as the following:

struct IpMreq
  imr_multiaddr : InAddr
  imr_interface : InAddr
end

struct Ipv6Mreq
  ipv6mr_multiaddr : In6Addr
  ipv6mr_interface : UInt
end

Please double check all bindings, some, like the OpenBSD bindings, are invalid —it's better to not have them than having wrong values (can lead to segfaults).

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

ping @ysbaddaden / @straight-shoota I've made those changes.
I believe the bindings are correct, what's wrong with the OpenBSD bindings?

I was looking in

src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Feb 27, 2019

I can't really review. I don't know anything about multicast —for example I have no idea what "hops" is.

I looked at how Ruby handles that, but it doesn't have any high-level interface for multicast in stdlib, so it doesn't help either :(

Changes were applied, and I don't know anything about multicast

@wontruefree

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

This is really cool to see in Crystal. Thanks for working on this.

@straight-shoota
Copy link
Member

left a comment

The two methods look good. Maybe there should be add an explicit note in the docs that these methods will raise when called with a non-matching address?

src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Great feedback @straight-shoota - I've implemented all those changes.

src/socket/udp_socket.cr Outdated Show resolved Hide resolved
spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@straight-shoota so not sure how to resolve the Darwin tests for IPv6.

I dev on macOS so can confirm that setting the default multicast interface on Darwin 18.2.0 does not work, as described in this bug: https://lists.apple.com/archives/darwin-kernel/2014/Mar/msg00012.html

However I can join the group successfully on my laptop (with IPv6) using crystal so I assume it's either an older version running on circle ci or possibly a permissions thing? not sure..

I see there is a similar issue with the it "sends broadcast message" do test for darwin sockets, so I think it's probably related (as i can send broadcast messages from macOS fine)

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Yeah, the CI uses xcode version 9.0 which runs on macOS 10.12.6 / Darwin 16.7.0. The bug has probably been fixed somewhere in between. We could consider simply updating the CI, but I'm not sure whether that's possible or desired. We aim to support macOS 10.7+ and the specs should work on all supported versions.

So for now, it's probably okay to simply disable the spec for #multicast_interface with IPv6 on darwin. But the rest of the spec should run on this platform!

I'd suggest to structure the tests like this, possibly even separating the individual methods (loopback, hops, join/leave) into separate specs.

each_ip_family do 
  # ...
  it "can join and transmit to multicast groups" do
    # setup
    # multicast_loopback
    # multicast_hops
    # {join,leave}_group + send/receive
  end
end
describe "#multicast_interface" do
  it "on IPv4" do
    # setup ipv4
    udp.multicast_interface Socket::IPAddress.new("0.0.0.0", 0)
    # send/receive test
  end
  {% unless flag?(:darwin) %}
    it "on IPv6" do
      # setup ipv6
      udp.multicast_interface 0
      # send/receive test
    end
  {% end %}
end      

We could consider activating the darwin spec based on the actual OS version. But for this it would be great to know when this was fixed.

leave_group is currently also missing from the specs, please add some expectation for that. Should also make sure that send/receive doesn't work after leaving.

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@straight-shoota all specs are passing for all platforms except Darwin IPv6 which displays as pending now.
image

I've added the test for group leave.

multicast_interface, group_join and group_leave IPv6 don't work on the CI version of Darwin unfortunately.
I think the fix for group_join and group_leave was introduced in late 2017: apple/darwin-xnu@0a798f6#diff-e2c4bcf26a9843a55acac3eb88326083

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@straight-shoota what do you think? Ready for merge?
Would love to see this in the next release

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Please separate the specs and make sure that the working specs run on darwin as suggested in my previous comment.

I think I'd also prefer to run all specs on recent darwin versions. This would just need to be a dynamic check based on the output of sw_vers (or similar) combined with flag?(:darwin). This would be the best solution, even without exactly pinpointing when it was fixed.

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@straight-shoota was just looking at this again.
Having no luck using sw_vers to determine if this version of OSX supports supports what. Have a feeling this issue lies not in the OS level code, but in the user level libraries supplied by XCode.

So I was thinking:

  1. Would it be OK if the code in the current state is accepted?
    • One test is pending: IPv6 multicast on Darwin
  2. I open a new pull request to implement getifaddrs
    • This will allow me to implement the missing behaviour of Darwin manually
    • I can fix IPv6 on OSX as part of that pull request - otherwise I feel like it's going to make this pull request very messy

(Note:: the current spec failure is an unrelated test)

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Yes of course, it's fine to simply skip the spec on darwin for now if a dynamic condition is too complicated.`

But as far as I understand it, only #multicast_interface fails, so the other specs should be executed on darwin with IPv6. A structure as proposed in #7423 (comment) would be ideal for this.

And please rebase on master, this should fix the unrelated spec failure.

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@straight-shoota it works on the latest version of OSX with the latest XCode libraries (I assume).

This is a minimal reproduction that can you can use to test on your laptop if you have OSX
(this works for me - but not on CircleCI)

require "socket"

struct Socket::IPAddress
  def addr6
    @addr6
  end
end

lib LibC
  struct Ipv6Mreq
    ipv6mr_multiaddr : LibC::In6Addr
    ipv6mr_interface : LibC::UInt
  end
end

s = UDPSocket.new Socket::Family::INET6
s.bind "::", 0
# multicast_interface selection
#s.setsockopt 9, 1, 41
#s.getsockopt 9, 41
sock = Socket::IPAddress.new("ff02::102", 2000)
addr = sock.addr6.not_nil!
  
req = LibC::Ipv6Mreq.new
req.ipv6mr_multiaddr = addr
req.ipv6mr_interface = 0
# Join IPv6 group
s.setsockopt 12, req, 41
Fiber.yield
s.send("testing", sock)
s.receive[0]

Thanks, I'll rebase.

@stakach stakach force-pushed the stakach:patch-4 branch 2 times, most recently from 2f09fd7 to ff76205 Mar 20, 2019

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@straight-shoota I didn't split the tests as none of the IPv6 multicast functions work on CircleCI version of Darwin or the build tools running on it.
I added this check, which should have worked based on my setup (you can confirm if you have OSX with the code I posted above)

I'll implement getifaddrs which should allow us to use either IP address or index for both IPv4 and v6 multicast_interface selection as well as working around the limitations of IPv6 on darwin.

Given not many people will be using multicast_interface selection and IPv6 multicast works on the latest versions of OSX (even if it's not being tested currently) I think this pull request is good to go

spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

@straight-shoota what do we think?

@straight-shoota
Copy link
Member

left a comment

Looks good, just a few details 👍

spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
spec/std/socket/udp_socket_spec.cr Outdated Show resolved Hide resolved
src/socket/address.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@straight-shoota I've implemented all those requested changes. Thank you for your patience btw.

@asterite

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@stakach it's actually your patience that we must thank, others would have probably abandoned this PR already :-)

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

need me to do anything more for this pull to be merged?

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Would be great if you could rebase against current master, to make sure it still works fine with 0.28.0-dev.

@stakach stakach force-pushed the stakach:patch-4 branch from 1923cb7 to 16430a4 Apr 26, 2019

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

@straight-shoota looking good :)

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@ysbaddaden @asterite would one of you be available to review this pull?

@stakach

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

The spec error for linux32 seems unrelated to this pull

src/socket/udp_socket.cr Outdated Show resolved Hide resolved
@stakach

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@asterite spare a moment for a final once over?

Stephen von Takach

@stakach stakach force-pushed the stakach:patch-4 branch from 5b47bd2 to 8507a88 May 17, 2019

@Sija

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

Merge 🕦?

@bcardiff bcardiff added this to the 0.29.0 milestone Jun 4, 2019

@bcardiff bcardiff merged commit c94be8b into crystal-lang:master Jun 4, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks @stakach and all reviewers involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.