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.

Sign up for GitHub

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

Handle ECONNABORTED in accept #250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle ECONNABORTED in accept #250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44debec
Handle ECONNABORTED in accept
reynir Dec 16, 2020
8b5fd86
Add CHANGES entry
reynir Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
# Unreleased

## Fixed

- Handle ECONNABORTED in accept (#250)

# 0.20.0

## Added
Expand Down
11 changes: 8 additions & 3 deletions opium/src/app.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,14 @@ let run_unix_multicore ?middlewares ~host ~port ~jobs handler =
(let+ () = Lwt_unix.bind socket listen_address in
Lwt_unix.listen socket (Lwt_unix.somaxconn () [@ocaml.warning "-3"]));
let rec accept_loop socket instance =
let* socket', sockaddr' = Lwt_unix.accept socket in
Lwt.async (fun () -> connection_handler sockaddr' socket');
accept_loop socket instance
Lwt.try_bind
(fun () -> Lwt_unix.accept socket)
(fun (socket', sockaddr') ->
Lwt.async (fun () -> connection_handler sockaddr' socket');
accept_loop socket instance)
(function
| Unix.Unix_error (Unix.ECONNABORTED, _, _) -> accept_loop socket instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to handle these as well:

      | Unix.Unix_error (Unix.EBADF, _, _)
      | Unix.Unix_error (Unix.ENOTCONN, _, _)
      | Unix.Unix_error (Unix.ECONNREFUSED, _, _)
      | Unix.Unix_error (Unix.ECONNRESET, _, _)

I'm curious how Lwt treats errors in establish_server_with_client_socket also. It'd be good to have the same behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me Lwt.establish_server_with_client_socket doesn't handle ECONNABORTED?! https://github.com/ocsigen/lwt/blob/master/src/unix/lwt_io.ml#L1599

I think we want to fail on EBADF still. I don't find ENOTCONN, ECONNREFUSED or ECONNRESET in the list of errors in accept(2). We should probably handle ECONNRESET at least where it might raise. I will look into that. Is ECONNREFUSED not only relevant for clients?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't find ENOTCONN, ECONNREFUSED or ECONNRESET

Indeed, we don't need to care about those then, let's only consider ECONNABORTED 👍

It seems to me Lwt.establish_server_with_client_socket doesn't handle ECONNABORTED?!

That's surprising.. Can you reproduce the crash consistently? If so, does it reproduce with the single-core version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't have a clue how to reproduce it. I exposed the opium server running on a FreeBSD machine to the internet. After 1-2 weeks I had the crash. It was running single-core.

I reported the same for establish_server upstream: ocsigen/lwt#829

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah! Let's wait and see what Lwt folks say about it then, we'll implement the same fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

Making it mandatory was probably not a good idea, I'll remove that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not able to pin this branch due to the conf-libev dependency. I'll pin to an earlier version with my path..

I'll try this on my freebsd install later, but I think libev is available for install on freebsd as a port https://www.freshports.org/devel/libev

I think once that's installed conf-libev should install successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think libev is available on FreeBSD, I just don't have permissions on that box to install it on that box :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think libev is available on FreeBSD, I just don't have permissions on that box to install it on that box :-)

Makes sense. That said, I think it'd be nice to not enforce a requirement on conf-libev in the opium package :-) I'm ➕ 1 with @tmattio on removing it as a dependency. The lwt docs mention libev and we should allow the end-user to decide if they can/can't use libev on their platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using opium 0.19.0 and the lwt patch I was not able to reproduce after 1000 tries \o/ ocsigen/lwt#830 (comment)

| e -> Lwt.fail e)
in
for i = 1 to jobs do
flush_all ();
Expand Down