Handle ECONNABORTED in accept #250
Conversation
| Lwt.async (fun () -> connection_handler sockaddr' socket'); | ||
| accept_loop socket instance) | ||
| (function | ||
| | Unix.Unix_error (Unix.ECONNABORTED, _, _) -> accept_loop socket instance |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah! Let's wait and see what Lwt folks say about it then, we'll implement the same fix
There was a problem hiding this comment.
Making it mandatory was probably not a good idea, I'll remove that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think libev is available on FreeBSD, I just don't have permissions on that box to install it on that box :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Using opium 0.19.0 and the lwt patch I was not able to reproduce after 1000 tries \o/ ocsigen/lwt#830 (comment)
|
I think this PR is still relevant for |
I had an opium server crash with an error
Fatal error: exception Unix.Unix_error(Unix.ECONNABORTED, "accept", ""). This PR handles this error case.