Skip to content

Add cancelation on tcpip.stack-socket#443

Merged
dinosaure merged 4 commits into
masterfrom
cancelation
Mar 26, 2021
Merged

Add cancelation on tcpip.stack-socket#443
dinosaure merged 4 commits into
masterfrom
cancelation

Conversation

@dinosaure
Copy link
Copy Markdown
Member

@dinosaure dinosaure commented Mar 22, 2021

This PR wants to fix an issue about cancelation of listening socket. You can see the error on this CI output:
https://opam.ci.ocaml.org/github/ocaml/opam-repository/commit/661b80c8d56556cd711fb2e8f7a1c0edae56bf15

I would like to have a review to incorporate it into 6.1.0 and it should close the issue #437. (/cc @talex5 & @hannesm)

@dinosaure dinosaure requested review from hannesm and talex5 March 22, 2021 17:16
@hannesm
Copy link
Copy Markdown
Member

hannesm commented Mar 22, 2021

I'm not sure I understand the patch. What is the goal in semantics that you try to achieve?

Why are there some changes to V4.listen_tcpv4 (but none to listen_udpv4)? What about the V6 and the V4V6 modules in the same file?

Is it possible to formulate a test case with the desired semantics (to see what is failing at the moment, and what the intention is)?

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Mar 22, 2021

What is the advantage of using a Lwt_switch.t here? Wouldn't a Lwt_condition.t be sufficient?

@dinosaure dinosaure force-pushed the cancelation branch 2 times, most recently from 12b667c to f235dc2 Compare March 22, 2021 18:49
@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Mar 22, 2021

Why are there some changes to V4.listen_tcpv4 (but none to listen_udpv4)? What about the V6 and the V4V6 modules in the same file?

Yes, I forget to apply such change on all stacks. Now it's fixed by a forced-push.

I'm not sure I understand the patch. What is the goal in semantics that you try to achieve?

Currently, the master socket used to listen is not properly closed which leads an error when you want to launch multiple times sequentially a server on a specific port. Such situation mostly appears when we do some tests and I got several times the EADDRINUSE. The reproducibility of such situation is hard and seems intrinsic to the underlying operating system used and it seems the case on the opam-ci.

What is the advantage of using a Lwt_switch.t here? Wouldn't a Lwt_condition.t be sufficient?

This is where @talex5 should have a better explanation than me.

@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Mar 24, 2021

According to the review of @talex5 and @hannesm, I pushed-force the new version of the cancellation without Lwt_switch.t.

talex5
talex5 reviewed Mar 24, 2021
View reviewed changes
Comment thread src/stack-unix/tcpip_stack_socket.ml Outdated
@dinosaure dinosaure requested a review from talex5 March 24, 2021 19:18
talex5
talex5 reviewed Mar 25, 2021
View reviewed changes
Comment thread src/stack-unix/tcpip_stack_socket.ml Outdated
Comment thread src/stack-unix/tcpip_stack_socket.ml Outdated
Comment thread src/stack-unix/tcpip_stack_socket.ml Outdated
dinosaure and others added 2 commits March 25, 2021 16:27
@dinosaure @talex5
Update src/stack-unix/tcpip_stack_socket.ml
4fcb3a6 
Co-authored-by: Thomas Leonard <talex5@gmail.com>
@dinosaure
Apply @talex5's review
509cb2c
@dinosaure
Copy link
Copy Markdown
Member Author

dinosaure commented Mar 26, 2021

I think we are ready for a release 👍 .

@dinosaure dinosaure merged commit f0b0094 into master Mar 26, 2021
@dinosaure dinosaure mentioned this pull request Mar 26, 2021
Closed
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 26, 2021
@dinosaure
[new release] tcpip (6.1.0)
dbbe1e2 
CHANGES:

* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 26, 2021
@dinosaure
[new release] tcpip (6.1.0)
8dc4803 
CHANGES:

* checksum stubs: Drop `caml_` from their name (@hannesm, mirage/mirage-tcpip#445)
* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
@dinosaure dinosaure mentioned this pull request Mar 29, 2021
Merged
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 29, 2021
@dinosaure
[new release] tcpip (6.1.0)
26907b3 
CHANGES:

* checksum stubs: Drop `caml_` from their name (@hannesm, mirage/mirage-tcpip#445)
* Add cancelation on `tcpip.stack-socket` (@dinosaure, @talex5, @hannesm, mirage/mirage-tcpip#443)
* Ensure that listen really binds the given socket before
  creating a task on `tcpip.stack-socket` (@dinosaure, @hannesm, mirage/mirage-tcpip#439)
* Add `ppx_cstruct` as a dependency (@hannesm, @dinosaure, mirage/mirage-tcpip#439)
* Upgrade to ocamlformat.0.17.0 (@dinosaure, mirage/mirage-tcpip#442)
* Drop the support of OCaml 4.08.0 (@dinosaure, mirage/mirage-tcpip#442)
* Use the usual layout to compile freestanding C stubs and link them to
  a Solo5 unikernel (@dinosaure, @hannesm, mirage/mirage-tcpip#441)
  **breaking changes**
  C stubs are prepended by `mirage_`. Symbols such as checksum's
  symbols are `caml_mirage_tcpip_*` instead of `caml_tcpip_*`
  `tcpip.unix` is a fake sub-package and user does not it anymore, he can
  safely remove it from its project.
* Conflict with `< ocaml-freestanding.0.4.1` (@hannesm, mirage/mirage-tcpip#441)
@MagnusS MagnusS mentioned this pull request Mar 30, 2021
Closed
@hannesm hannesm deleted the cancelation branch July 19, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@talex5 talex5 talex5 left review comments

@hannesm hannesm Awaiting requested review from hannesm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dinosaure @hannesm @talex5