Add a initial engine based on luv

[ulrikstrid]

This is meant to be a conversation starter above all else. The code is pretty naive at this point.

It creates a new engine that is based on luv (lovingly called the luv engine).
This could be a nice migration path to move the whole unix module to be using libuv which is very well maintained and tested since it's been used by node for years.

Locally I have the same test failures I have with the libev engine.

[ulrikstrid]

I'm outside of my comfort zone with the file_descr stuff but I think we might be able to pick the solution from here: aantron/luv@705ec14

[aantron]

I should probably restore that code to Luv, as it has use outside Lwt for those that want to use Luv together with some Unix code. In the meantime, though, you're welcome to borrow from it as needed. We can adjust Lwt later, if Luv gets that code again.

[ulrikstrid]

It looks like the code is using some internal code that is not exposed outside of Luv, specifically C.Types.Os_fd.t. Not sure how to handle that.

[aantron]

It looks like the code is using some internal code that is not exposed outside of Luv, specifically C.Types.Os_fd.t. Not sure how to handle that.

C.Types.Os_fd.t is a Ctypes binding to libuv's os_fd_t. Since Lwt is not using Ctypes (at the moment), it can just be replaced by int.

It might be more work-efficient, however, to simply not support Windows during this PR. We will merge the PR in a non-intrusive way (as an optional package in this repo), so the lack of Windows support won't affect anyone. Then, I'll restore the Unix-libuv fd conversions in Luv (probably in a helper package), and we can simply call those from Lwt to help get the Windows support working. As the code matures more and more, we can consider moving it from the optional package into main Lwt. I'll post a suggested roadmap for doing so and follow-on work that is enabled by the libuv integration, and we will discuss with @raphael-proust and @avsm.

It looks like the Luv test cases will also benefit from the conversions:

(* Until https://github.com/libuv/libuv/pull/1498. This implementation will not
   work on Windows. One can be provided, but hopefully the PR lands first. *)
let unix_fd_to_file : Unix.file_descr -> Luv.File.t =
  Obj.magic

The Luv tester also shows that there is precedent for getting away with Obj.magic on Unix, at least for non-production code, which is what this integration will be in its first days after merging if following my suggestion above :)

[aantron]

So focusing on Unix only, at the moment, for me locally, the Unix tester passes, but the React and PPX testers hang.

In Travis, I see errors like this:

(cd _build/default/test/ppx && ./main.exe)
Testing library 'ppx'...
Fatal error: exception Failure("Could not register fd for read polling, this is probably a error in Lwt, please open a issue on the repo. \nError message: operation not permitted")
Raised at file "stdlib.ml", line 29, characters 17-33
Called from file "src/unix/lwt_engine.ml", line 88, characters 15-37
Called from file "src/unix/lwt_engine.ml", line 73, characters 29-47
Called from file "src/core/lwt_sequence.ml", line 132, characters 31-47
Called from file "src/unix/lwt_engine.ml", line 72, characters 4-112
Called from file "src/unix/lwt_engine.ml", line 472, characters 19-36
Called from file "test/test.ml", line 268, characters 2-42
Called from file "test/ppx/main.ml", line 165, characters 8-32
        main alias test/core/runtest

and similar errors for React.

[ulrikstrid]

So I think I addressed your comments now.

The next step is to break it out of lwt-unix so that not everyone needs to depend on Luv at this point (even if I think they should 😉). How do you want me to do that?

[aantron]

So I think I addressed your comments now.

👍

The next step is to break it out of lwt-unix so that not everyone needs to depend on Luv at this point (even if I think they should 😉). How do you want me to do that?

Create a new directory, say, src/unix/luv, with its own dune file that declares a library called lwt_luv (or any other suitable name) that depends on luv and lwt.unix. Then move the libuv engine class into a new .ml file in that directory (probably with an .mli file for it, too). Then add lwt_luv.opam with some metadata for that library.

Then create test/unix/luv_main.ml as a copy of main.ml, but insert the call to set the engine to libuv into it. Remove it from main.ml. Declare the luv_main executable in test/unix/dune, and make it depend on lwt_luv. Don't add it to the runtest alias as we don't want Dune to run it on Windows as part of our AppVeyor build. Instead, we will run it manually only in Travis. After this line in .travis.yml:

lwt/.travis.yml

Line 100 in 1157930

dune runtest --force

...insert something like dune exec test/unix/luv_main.exe.

Copying main.ml is probably not ideal in the long term; we'd want to list out the test suites in one place instead. However, I hope this separate packaging is only temporary, and eventually main.ml will itself become the libuv tester if Lwt starts using libuv by default.

[aantron]

Also we probably won't ever release lwt_luv.opam, as we'd rather see if we will merge the libuv support into the main Lwt instead. So it doesn't have to be neat. It just has to have the dependency metadata to make Travis and/or contributors install Luv.

[ulrikstrid]

I think I did the breakout correctly, I don't have my opam drivers license yet so might have missed something there.

[aantron]

I think the most important missing thing is triggering the new luv_main.exe tester from .travis.yml.

[ulrikstrid]

Just letting you know I'm still working on this. I'm currently trying to debug the Linux failure/running forever.
Sometimes Lwt_main.run gets called in the child process when it should quit and then it hangs forever.

[ulrikstrid]

There are still failing tests but I think the major stuff works now and I'm not sure it's worth it to fix the rest until we swap the rest out

[aantron]

This is in my queue for reviewing for merge. Should have time soon.

[aantron]

Want to link again to this failure :) https://travis-ci.com/github/ocsigen/lwt/jobs/439876055#L414

[aantron]

Merging this, despite Travis CI not running at all. It (mostly) works locally on my system.

1. This doesn't affect "base" Lwt — almost all the new code is in Luv_engine.ml, in the separate package lwt_luv.opam, which we don't intend to release for the time being (but people can pin). The only exception to this is the addition of method fork to the base Lwt_engine class, which does nothing in that base class — though it should be mentioned in the changelog of 5.4.0, cc @raphael-proust.
2. The tests fail sporadically. We can stabilize this in and during follow-on work. Some of the occasional failures are due to too-strict timing tests. It's better to look at all that later, rather than risk excessive bit rot in this PR.

We will go from here as described in #813 "A Roadmap for Porting lwt.unix to libuv."

Thank @ulrikstrid and reviewers!