net: add Net.with_tcp_connect #302
Conversation
There was a problem hiding this comment.
Ideally, Eio.Net.Unix_error is an implementation level exception which could be hidden from lib eio users. Question to maintainers - should this exn be hidden? At the moment there is net.mli file which prevents this from being hidden at the eio level.
It can't be hidden because other people need to use it (either to create custom networks that raise it, or custom connection logic that catches it). You could just use Connection_failure for this too.
The usual pattern in Eio is:
exception Connection_failure of exnThen the full details (the Unix_error or whatever) are available for logging, etc.
lib_eio/net.mli
Outdated
| @@ -12,6 +12,8 @@ | |||
| *) | |||
|
|
|||
| exception Connection_reset of exn | |||
| exception Unix_error of string * string * string | |||
| exception Connection_failure of (string * string) | |||
There was a problem hiding this comment.
No need for an extra tuple here.
| exception Connection_failure of (string * string) | |
| exception Connection_failure of string * string |
But what are these strings, anyway?
There was a problem hiding this comment.
(host, port/service)
There was a problem hiding this comment.
The exception is Connection_failure of exn now.
tests/network.md
Outdated
|
|
||
| ## with_tcp_connet | ||
|
|
||
| <!-- $MDX non-deterministic=command --> |
There was a problem hiding this comment.
You can pass in a mock network and make this test actually run. We already tested that DNS lookups and connecting work, so we only need to test the logic of trying them in sequence here.
There was a problem hiding this comment.
It seems we can't mock this just yet, since we also need to be able to mock clock due to - I think - usage of Eio.Time.timeout function.
There was a problem hiding this comment.
I've added a mock clock now in #328.
bikallem
force-pushed
the
with_connect
branch
3 times, most recently
from
54d0546 to
9c844b4
Compare
`Net.with_tcp_connect` takes a domain-name/port/service and attempts to establish connection to it. IP addresses for the host are tried one after the other while preferring/trying IPv6 protocol addresses first if available. We also introduce two new exceptions: 1. Invalid_host - raised when a connection couldn't be established for any of the addresses defined for a given host/port/service. 2. Unix_error - introduced to mimic Unix.Unix_error without depending on "unix" library.
talex5
force-pushed
the
with_connect
branch
2 times, most recently
from
4d63e4c to
3a4eb47
Compare
talex5
force-pushed
the
with_connect
branch
from
3a4eb47 to
bd160c9
Compare
There was a problem hiding this comment.
I rebased this on main and updated it to use the new Eio.Time.Timeout and Eio_mock.Clock modules. The tests no longer require a real network and so can always be run.
I also removed the Net.Unix_error exception (using Connection_failure instead) to make it more generic, and made with_tcp_connect pass along the (final) exception from the network, so you can see why it failed.
If you're happy with that, it's ready to merge.
lib_eio/net.ml
Outdated
| exception Unix_error of string * string * string | ||
| (* Unix.Unix_error without depending on "unix" *) | ||
|
|
There was a problem hiding this comment.
| exception Unix_error of string * string * string | |
| (* Unix.Unix_error without depending on "unix" *) |
tests/network.md
Outdated
|
|
||
| ## with_tcp_connet | ||
|
|
||
| <!-- $MDX non-deterministic=command --> |
There was a problem hiding this comment.
I've added a mock clock now in #328.
|
Thanks. I did a pass-through and the changes look good to me. 👍 |
|
Thanks! |
CHANGES: Changes: - Update to OCaml 5.0.0~beta1 (@anmonteiro @talex5 ocaml-multicore/eio#346). - Add API for seekable read/writes (@nojb ocaml-multicore/eio#307). - Add `Flow.write` (@haesbaert ocaml-multicore/eio#318). This provides an optimised alternative to `copy` in the case where you are writing from a buffer. - Add `Net.with_tcp_connect` (@bikallem ocaml-multicore/eio#302). Convenience function for opening a TCP connection. - Add `Eio.Time.Timeout` (@talex5 ocaml-multicore/eio#320). Makes it easier to pass timeouts around. - Add `Eio_mock.Clock` (@talex5 ocaml-multicore/eio#328). Control time in tests. - Add `Buf_read.take_while1` and `skip_while1` (@bikallem ocaml-multicore/eio#309). These fail if no characters match. - Make the type parameter for `Promise.t` covariant (@anmonteiro ocaml-multicore/eio#300). - Move list functions into a dedicated submodule (@raphael-proust ocaml-multicore/eio#315). - Direct implementation of `Flow.source_string` (@c-cube ocaml-multicore/eio#317). Slightly faster. Bug fixes: - `Condition.broadcast` must interlock as well (@haesbaert ocaml-multicore/eio#324). - Split the reads into no more than 2^32-1 for luv (@haesbaert @talex5 @EduardoRFS ocaml-multicore/eio#343). Luv uses a 32 bit int for buffer sizes and wraps if the value passed is too big. - eio_luv: allow `Net.connect` to be cancelled (@talex5 @nojb ocaml-multicore/eio#311). - eio_main: Use dedicated log source (@anmonteiro ocaml-multicore/eio#326). - linux_eio: fix kernel version number in log message (@talex5 @nojb ocaml-multicore/eio#314). - Account for stack differences in the socketpair test (issue ocaml-multicore/eio#312) (@haesbaert ocaml-multicore/eio#313). Documentation: - Add Best Practices section to README (@talex5 ocaml-multicore/eio#299). - Documentation improvements (@talex5 ocaml-multicore/eio#295 ocaml-multicore/eio#337).
Net.with_tcp_connecttakes a domain-name/port/service and attempts toestablish connection to it. IP addresses for the host are tried one
after the other while preferring/trying IPv6 protocol addresses first
if available.
We also introduce two new exceptions:
for any of the addresses defined for a given host/port/service.
on "unix" library in the "lib_eio" which is (should?) be free from "unix" lib dependency if I understand the design/architecture of "eio".
Open Issues
Ideally,
Eio.Net.Unix_erroris an implementation level exception which could be hidden from libeiousers. Question to maintainers - should this exn be hidden? At the moment there isnet.mlifile which prevents this from being hidden at theeiolevel.