Allow datagram_socket to be created without address by bikallem · Pull Request #360 · ocaml-multicore/eio

bikallem · 2022-10-31T13:31:14Z

This PR allows to create UDP datagram_socket without passing in the socket address. Being able to create datagram_socket without socket address is useful in udp client applications where it is convenient to send data to UDP severs without having to figure out the udp client address first.

Fixes #345 #341

bikallem · 2022-10-31T13:39:15Z

/cc @haesbaert

tests/network.md

haesbaert · 2022-10-31T14:22:14Z

Code LGTM, that's in the direction I was thinking as well, it feels a bit weird that datagram_socket is either a sockaddr or a Udp4, but I can't think of a better way to do it without changing everything.

bikallem · 2022-11-02T16:51:28Z

@haesbaert could you please approve if you are happy with the PR. I want to use this for my ocaml-dns PR (mirage/ocaml-dns#312)

haesbaert

LGTM maybe @talex5 wants to have a look as well

lib_eio/net.ml

lib_eio/net.mli

talex5 · 2022-11-03T11:52:01Z

lib_eio_linux/eio_linux.ml

      udp_socket sock
+    | (`UdpV4 | `UdpV6) as s ->
+      let socket_fd = Unix.socket ~cloexec:true (socket_domain_of s) Unix.SOCK_DGRAM 0 in
+      let sock = FD.of_unix ~sw ~seekable:false ~close_unix:true socket_fd in


Does is matter that the socket options are different in this case?

(Aside: why is SO_REUSEPORT on by default? That seems like a dangerous option, allowing two programs to listen on the same port by mistake! )

Good point, I wonder if it actually has any effect if you don't bind(2) ?

Not sure about SO_REUSEPORT myself. I think we should remove these options altogether and leave it to the user of the lib to set them if they need it after #322 is implemented. Should both of reuseport and resuseaddress settings be removed? in both TCP and UDP?

Let's leave this to PR which will address #322

I think it's fine and even desirable to have reuse_{addr,port} as an optional argument, but it should be default off, I'd add the optional arguments to datagram_socket to mimick the behaviour of listening_socket (default off).
This would be a breaking change as old users of datagram_socket Sockaddr would now get default off, but I think that's better than the old behavior.

I have opened #362 to discuss and track this issue.

Does is matter that the socket options are different in this case?

Yes, Udp4 creates socket domain of Unix.PF_INET and Udp6 creates socket domain of Unix.PF_INET6.

EDIT: Do you mean the pattern matching? I suppose not but it is better to be explicit, no?

I meant I wasn't sure if the reuse options were useful even without a fixed source address. I guess it is probably best to ignore them, as you did.

tests/network.md

bikallem · 2022-11-03T16:43:42Z

@talex5 I have addressed the review comments.

bikallem · 2022-11-04T10:53:12Z

@haesbaert I have now added ?(reuse_addr=false) and ?(reuse_port=false) to Eio.Net.datagram_socket function.

Removed the last IPv6 test since it was identical to an earlier test.

Fix problem that UDP sockets were not created close-on-exec.

talex5

I pushed some more changes (to be squashed if accepted):

I removed some duplication in the test code (and removed a duplicate test).
I changed the socket creation code to share the bits that were the same. That revealed that the old path wasn't setting close-on-exec.
I removed the new functions from eio_unix. This is intended for users integrating with Unix, rather than as a place for sharing common code between backends, and these functions were quite confusing (they do nothing if passed false), and also not shared.

If that looks OK, I'm happy for it to be merged.

bikallem · 2022-11-04T11:41:08Z

LGTM. 👍

@talex5

CHANGES: API changes: - Unify IO errors as `Eio.Io` (@talex5 ocaml-multicore/eio#378). This makes it easy to catch and log all IO errors if desired. The exception payload gives the type and can be used for matching specific errors. It also allows attaching extra information to exceptions, and various functions were updated to do this. - Add `Time.Mono` for monotonic clocks (@bikallem @talex5 ocaml-multicore/eio#338). Using the system clock for timeouts, etc can fail if the system time is changed during the wait. - Allow datagram sockets to be created without a source address (@bikallem @haesbaert ocaml-multicore/eio#360). The kernel will allocate an address in this case. You can also now control the `reuse_addr` and `reuse_port` options. - Add `File.stat` and improve `Path.load` (@haesbaert @talex5 ocaml-multicore/eio#339). `Path.load` now uses the file size as the initial buffer size. - Add `Eio_unix.pipe` (@patricoferris ocaml-multicore/eio#350). This replaces `Eio_linux.pipe`. - Avoid short reads from `getrandom(2)` (@haesbaert ocaml-multicore/eio#344). Guards against buggy user code that might not handle this correctly. - Rename `Flow.read` to `Flow.single_read` (@talex5 ocaml-multicore/eio#353). This is a low-level function and it is easy to use it incorrectly by ignoring the possibility of short reads. Bug fixes: - Eio_luv: Fix non-tail-recursive continue (@talex5 ocaml-multicore/eio#378). Affects the `Socket_of_fd` and `Socketpair` effects. - Eio_linux: UDP sockets were not created close-on-exec (@talex5 ocaml-multicore/eio#360). - Eio_linux: work around io_uring non-blocking bug (@haesbaert ocaml-multicore/eio#327 ocaml-multicore/eio#355). The proper fix should be in Linux 6.1. - `Eio_mock.Backend`: preserve backtraces from `main` (@talex5 ocaml-multicore/eio#349). - Don't lose backtrace in `Switch.run_internal` (@talex5 ocaml-multicore/eio#369). Documentation: - Use a proper HTTP response in the README example (@talex5 ocaml-multicore/eio#377). - Document that read_dir excludes "." and ".." (@talex5 ocaml-multicore/eio#379). - Warn about both operations succeeding in `Fiber.first` (@talex5 ocaml-multicore/eio#358, reported by @iitalics). - Update README for OCaml 5.0.0~beta2 (@talex5 ocaml-multicore/eio#375). Backend-specific changes: - Eio_luv: add low-level process support (@patricoferris ocaml-multicore/eio#359). A future release will add Eio_linux support and a cross-platform API for this. - Expose `Eio_luv.Low_level.Stream.write` (@patricoferris ocaml-multicore/eio#359). - Expose `Eio_luv.Low_level.get_loop` (@talex5 ocaml-multicore/eio#371). This is needed if you want to create resources directly and then use them with Eio_luv. - `Eio_linux.Low_level.openfile` is gone (@talex5 ocaml-multicore/eio#378). It was just left-over test code.

bikallem mentioned this pull request Oct 31, 2022

Add eio backend for ocaml-dns client mirage/ocaml-dns#312

Closed

5 tasks

haesbaert reviewed Oct 31, 2022

View reviewed changes

tests/network.md Outdated Show resolved Hide resolved

haesbaert reviewed Oct 31, 2022

View reviewed changes

tests/network.md Outdated Show resolved Hide resolved

haesbaert reviewed Oct 31, 2022

View reviewed changes

tests/network.md Show resolved Hide resolved

haesbaert approved these changes Nov 3, 2022

View reviewed changes

talex5 reviewed Nov 3, 2022

View reviewed changes

bikallem added 3 commits November 3, 2022 16:13

net: allow to create udp socket without address

46ab470

udp: add udp test

af2fd57

udp: allow Eio.Net.Sockaddr.t in datagram_socket() without casting

bc9cfc3

bikallem mentioned this pull request Nov 3, 2022

Eio shouldn't set socket SO_REUSEPORT and SO_REUSEADDR options when creating sockets #362

Closed

bikallem and others added 5 commits November 4, 2022 11:01

udp: add reuse_addr and reuse_port to datagram_socket

8320f56

Simplify tests

53de440

Removed the last IPv6 test since it was identical to an earlier test.

Simplify luv code

921e3e4

Move socket options out of Eio_unix

b0cd160

Simplify eio_linux code

70042f5

Fix problem that UDP sockets were not created close-on-exec.

talex5 approved these changes Nov 4, 2022

View reviewed changes

bikallem merged commit a70865e into ocaml-multicore:main Nov 4, 2022

bikallem deleted the fix-udp branch November 4, 2022 11:45

haesbaert mentioned this pull request Nov 4, 2022

Can't use unbound UDP sockets #341

Closed

bikallem mentioned this pull request Nov 4, 2022

luv: implement luv reuseport and reuseaddr #363

Closed

bikallem mentioned this pull request Dec 2, 2022

EIO: Getting `Tls Failure (Fatal MACMismatch) mirleft/ocaml-tls#457

Closed

talex5 mentioned this pull request Dec 7, 2022

[new release] eio_main, eio_luv, eio_linux and eio (0.7) ocaml/opam-repository#22619

Merged

Conversation

bikallem commented Oct 31, 2022

Uh oh!

bikallem commented Oct 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haesbaert commented Oct 31, 2022

Uh oh!

bikallem commented Nov 2, 2022

Uh oh!

haesbaert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

talex5 Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

haesbaert Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

bikallem Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

bikallem Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

haesbaert Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

bikallem Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

bikallem Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talex5 Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bikallem commented Nov 3, 2022

Uh oh!

bikallem commented Nov 4, 2022

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

bikallem commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bikallem Nov 4, 2022 •

edited

Loading

bikallem commented Nov 4, 2022 •

edited

Loading