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

Lwt should depend on conf-libev #871

Open
talex5 opened this issue Jul 12, 2021 · 7 comments
Open

Lwt should depend on conf-libev #871

talex5 opened this issue Jul 12, 2021 · 7 comments

Comments

@talex5
Copy link
Contributor

talex5 commented Jul 12, 2021

Lwt defaults to using select, which eventually fails on most systems with:

Unix.Unix_error(Unix.EINVAL, "select", "")

This tends to result in services that pass their tests, but fail unpredictably in production, once FDs over FD_SETSIZE start being used.

Can we just put "conf-libev" {os != "win32"} in Lwt's opam file to fix this everywhere?

/cc @avsm

talex5 added a commit to talex5/ocurrent that referenced this issue Jul 12, 2021
Several services have failed due to Lwt using `select` by default.
Ideally this should be fixed in Lwt (see ocsigen/lwt#871),
but for now at least fix it for all OCurrent pipelines.
@avsm
Copy link
Collaborator

avsm commented Jul 12, 2021

I'm in favour of making this change as it's been an extremely common source of errors in using Lwt_unix in production over the years. @raphael-proust ?

@smorimoto
Copy link
Member

I completely agree with your opinions. Let's do it.

@raphael-proust
Copy link
Collaborator

The main issue I see with that is for project using Lwt without Unix. Basically, the projects using js_of_ocaml will have this additional enforced dependency that they don't need.

  1. I'm not sure that's many projects. Maybe a handful? cohttp-lwt-jsoo and joolog and maybe more??
  2. We could make a breaking change not in the API but in the packaging where we have an lwt-with-unix package that's lwt with base-unix installed (and the added proposed constraint) and a separate lwt-without-unix package that's lwt without base-unix installed. We'd need to add some conflicts and such. And it'd require changing a lot of other project's opam files.

We could also ignore this and assume that everyone can install libev even if they don't intend to use it, but I don't know that it's the best course of action.

@talex5
Copy link
Contributor Author

talex5 commented Jul 24, 2021

Why would lwt-without-unix be needed? I'd think just lwt and lwt-unix packages would be enough.

@raphael-proust
Copy link
Collaborator

Oh yes, I didnt meant these names literally and lwt would be a better name for it.

@aantron Do you know the historical reason why Lwt has an optional base-unix dependency rather than be made of two distinct packages? Do you know if this reason still applies? Do you foresee any issues with switching to two distinct packages?

As an interesting data-point, the set grep -L base-unix $(grep -l lwt packages/**/opam) is far from empty. I wonder how many of those have an implicit dependency to base-unix through another package.

@aantron
Copy link
Collaborator

aantron commented Jul 27, 2021

@aantron Do you know the historical reason why Lwt has an optional base-unix dependency rather than be made of two distinct packages? Do you know if this reason still applies? Do you foresee any issues with switching to two distinct packages?

Lwt was historically a single opam package with many depopts, most of which I factored out into their own repositories or at least packages when I was maintaining Lwt. AFAIK there is no good reason for this, it's just how things were done "back in the day" (and way before I started working on Lwt). It's likely a holdover from the "./configure" style of installation that was around before opam.

I assume that many packages will break if this is changed, especially if the "bigger" package is the one that gets renamed as @talex5 suggests. The safer way to do this is to essentially factor the core out of lwt into lwt-core or lwt-pure. But I also wonder how many projects would actually depend on that core.

The libev issue might be made obsolete by libuv port, though there is no progress on that yet.

@raphael-proust
Copy link
Collaborator

When next I work on Lwt, I will attempt to remove the optional dependency. I will probably attempt to have

  • lwt-core: the unix-less part
  • lwt-unix: the unix part

I will also attempt to have

  • lwt: a meta package that depends on lwt-core and depends on lwt-unix if base-unix is installed

This would be to ease transition and it would be intended to either be removed or more likely made to depend on both lwt-core and lwt-unix unconditionally. I'll experiment a bit when I work on it. In the meantime, suggestions welcome.

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

No branches or pull requests

5 participants