lwt icon indicating copy to clipboard operation
lwt copied to clipboard

Lwt should depend on conf-libev

Open talex5 opened this issue 2 years ago • 7 comments

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 avatar Jul 12 '21 08:07 talex5

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 ?

avsm avatar Jul 12 '21 08:07 avsm

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

smorimoto avatar Jul 12 '21 08:07 smorimoto

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.

raphael-proust avatar Jul 24 '21 10:07 raphael-proust

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

talex5 avatar Jul 24 '21 11:07 talex5

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.

raphael-proust avatar Jul 24 '21 13:07 raphael-proust

@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.

aantron avatar Jul 27 '21 14:07 aantron

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.

raphael-proust avatar Sep 07 '21 06:09 raphael-proust