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 listening_socket#listening_addr #555

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Conversation

mefyl
Copy link
Contributor

@mefyl mefyl commented Jun 11, 2023

This adds the ability to retrieve the address a socket is listening on. More than just a convenience, this is required to let the OS pick a random free port by passing 0 and then determine what port was actually picked.

The actual implementation returns None if the socket was closed, while the mock always returns None - I'm unsure whether this is an issue as I'm only just starting to dip my toes in eio.

@patricoferris
Copy link
Collaborator

Thanks! This looks sensible to me, I think the Eio_windows implementation would be the same as the rest using the Unix module.

As with other APIs I think a function wrapping the method call would be useful so people don't have to use # if they don't want to. This could then document the semantics of calling listening_addr.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks reasonable. It's probably better to raise an exception if it's closed (I'd consider it to be a bug to use a socket after closing it, not something you need to write code to check for).

The mock could take a ?listening_addr argument, with some suitable default dummy value (since most people won't care).

In any case, we should document this behaviour of passing 0 as the port to listen. Linux's bind manpage only seems to mention this in the errors section, under EADDRINUSE. https://stackoverflow.com/a/1077305/50926 has more information.

@avsm
Copy link
Contributor

avsm commented Jul 12, 2023

Does the bind to 0 port trick work on macOS? It's not documented there, and I remember encountering issues with this some years ago when trying it for Docker for Desktop...

@mefyl
Copy link
Contributor Author

mefyl commented Jul 17, 2023

Does the bind to 0 port trick work on macOS? It's not documented there, and I remember encountering issues with this some years ago when trying it for Docker for Desktop...

Looks like it does on some MacOS at least:

$ uname -a
Darwin mac1gitlabroutineco.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:19 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T8103 arm64
$ cat net.c 
#include <stdio.h>
#include <stdlib.h>
#include <netdb.h>

#define handle_error(msg) \
    do { perror(msg); exit(EXIT_FAILURE); } while (0)

int
main(int argc, char *argv[])
{
  int sock = socket(AF_INET, SOCK_STREAM, 0);
  if (sock == -1)
    handle_error("socket");
  struct sockaddr_in addr;
  addr.sin_family = AF_INET;
  addr.sin_addr.s_addr = htonl(INADDR_ANY);
  addr.sin_port = htons(0);
  if (bind(sock, (struct sockaddr *) &addr, sizeof addr) == -1)
    handle_error("bind");
  if (listen(sock, 5) == -1)
    handle_error("listen");
  socklen_t length = sizeof addr;
  if (getsockname(sock,(struct sockaddr *) &addr, &length) == -1) 
    handle_error("getsockname");
  printf("listening on %d\n", ntohs(addr.sin_port));
}
$ gcc net.c -o net
$ ./net
listening on 49176
$ ./net
listening on 49177
$ ./net
listening on 49178
$ ./net
listening on 49179
$ 

To be frank I always assumed it was part of POSIX, I'm surprised it's that sparsely documented. I've always used it as the standard way to listen on a random port, especially in test suites that were running on multiples architecture, including macs. But it looks indeed like it's not that clear cut.

@mefyl
Copy link
Contributor Author

mefyl commented Dec 26, 2023

May I humbly ask for a status on this ? It's something I rely upon a lot, especially in test suites. I don't mind porting it to the current API if we're willing to accept the feature.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good, but there are a load of accidental changes to CHANGES.md (not sure how it happened; .gitattributes should have prevented it).

lib_eio_linux/eio_linux.ml Outdated Show resolved Hide resolved
@talex5 talex5 mentioned this pull request Jan 15, 2024
@nojb
Copy link
Contributor

nojb commented Jan 15, 2024

Hello, I recently needed this (see #668); note that this should also support datagram (udp) sockets to be complete.

@talex5 talex5 force-pushed the main branch 2 times, most recently from 906ddd5 to b16c229 Compare January 15, 2024 09:43
@talex5 talex5 changed the title Add listening_socket#listening_addr. Add listening_socket#listening_addr Jan 15, 2024
Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good, once CI has finished. I squashed and rebased and updated to use_exn in the other places.

(Note: GitHub doesn't send notifications when you add commits, so it's helpful to post a comment when a PR is ready for review again.)

@nojb: I agree it would be good to have this for datagram sockets too, but I'm happy for that to be a separate PR.

@talex5 talex5 merged commit ccf1ba7 into ocaml-multicore:main Jan 15, 2024
3 of 4 checks passed
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
talex5 added a commit to talex5/opam-repository that referenced this pull request Jan 17, 2024
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

New features / API changes:

- Add `Eio.Executor_pool` (@SGrondin ocaml-multicore/eio#639, reviewed by @talex5).
  Provides an easy way to distribute jobs across domains.

- Add `Fiber.first ~combine` and `Fiber.n_any` (@SGrondin @talex5 ocaml-multicore/eio#587).
  Allows keeping both results in the case where multiple fibers succeed.

- Add `Eio_mock.Backend.run_full` with auto-advancing mock clock (@talex5 ocaml-multicore/eio#644, reviewed by @SGrondin).
  Simplifies testing of code using clocks.

- Add `Buf_write.printf` (@SGrondin @talex5 ocaml-multicore/eio#655).

- Add `Net.listening_addr` (@mefyl ocaml-multicore/eio#555, reviewed by @patricoferris @talex5).
  Useful to get the socket's address if the OS assigns it.

- Add `Promise.try_resolve` (@talex5 ocaml-multicore/eio#646).

- Remove `Cancel_hook_failed` exception (@talex5 ocaml-multicore/eio#640).
  Didn't seem to be used and broke dscheck.

Tracing:

- Improve tracing (@TheLortex @patricoferris @talex5 ocaml-multicore/eio#656).
  Trace cancellation contexts and OS operations, and simplify API.

- Add labels to switches (@talex5 ocaml-multicore/eio#661, reviewed by @SGrondin).

- `Fiber.all`: use the parent fiber (@talex5 ocaml-multicore/eio#665, reviewed by @SGrondin).
  Cleans up the traces a bit.

Performance:

- Faster and simpler `Lf_queue` (@talex5 ocaml-multicore/eio#647, based on work by @polytypic).

- Optimise `Flow.copy` with `Buf_read.as_flow` (@talex5 ocaml-multicore/eio#663, reviewed by @SGrondin, reported by @leostera).

Bug fixes:

- Fix handling of very long IO vectors (@talex5 ocaml-multicore/eio#653, reported by @Cjen1).

- eio_posix: use `caml_enter_blocking_section` in more places (@talex5 ocaml-multicore/eio#654, reviewed by @SGrondin).

- eio_posix: work around `caml_unix_alloc_sockaddr` bug (@talex5 ocaml-multicore/eio#651).

- Remove default backtrace from `Switch.fail` (@talex5 ocaml-multicore/eio#664).

Documentation:

- Organise eio.mli better (@talex5 ocaml-multicore/eio#667).

- Fix quoting of quotes in process error messages (@talex5 ocaml-multicore/eio#666, reviewed by @SGrondin).

- Mention Path module in File and Fs documentation (@talex5 ocaml-multicore/eio#659, requested by @clecat).

- Minor documentation updates (@SGrondin @talex5 ocaml-multicore/eio#670).

Build / internals:

- Allow closing synchronous streams (@talex5 ocaml-multicore/eio#641, reviewed by @SGrondin).
  This isn't currently exposed in the public interface.

- Fix non-idempotent tests (@SGrondin ocaml-multicore/eio#662).

- eio_windows: add explicit fmt dependency (@talex5 ocaml-multicore/eio#643).
@talex5
Copy link
Collaborator

talex5 commented Jun 29, 2024

To be frank I always assumed it was part of POSIX, I'm surprised it's that sparsely documented.

Apparently the bind 0 behaviour has just been added: https://sortix.org/blog/posix-2024/

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

5 participants