Initial Javascript browser backend

[patricoferris]

This PR started off as both a Node and Browser backend, I'll follow up with Node so now only the Browser backend is here.

This PR is initial support for Javascript backends in Eio with two new packages: eio_node and eio_browser. It's important to note that this is just a first step to getting full support and a releasable set of packages.

Implementation

Javascript, both in Node and the browser, are inherently single-threaded and event-driven. As far as I can tell this means there's no simple way to yield to the event loop, instead when you get to that point you either turn your computation into something that emits an event when it is done or you create a Javascript promise and await that. Both eio_node and eio_browser have opted for the latter and both make the assumption that they will not have domain manager support.

The implementations return a promise for the computation (an 'a Fut.t) which a user could then await in their program if they want to sequence it with something else. Both implementations keep track of pending IO operations using a counter (this need not be Atomic as we only have a single domain) and make sure to enqueue a "wakeup" if there is pending IO. In Node this wakeup is setImmediate which is called on the next event loop iteration (see https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/). In the browser this is not available so I've used a setTimeout with a 0 second argument.

Currently Eio_node only support Filesystem access to make the reviewing a little easier. It shouldn't be too difficult to add support for the rest of the stdenv pieces in follow-up PRs along with better cancellation support if possible. Nodejs is more or less a wrapper around Libuv so a lot can be taken from that.

Other node implementation ideas

It is a little annoying that the main loop returns an 'a Fut.t, but I couldn't work out a way to not make this the case. I had tried using two Worker Threads and synchronising with Atomics, but couldn't get that to work.

Running Locally

At the time of writing, there seems to be a few issues with dune/js_of_ocaml which I think are being fixed w.r.t to separate compilation. You will need to get a development version of dune with fixes like ocaml/dune#6828 and ocaml/dune#6714 but I still seem to have problems with the --enable=effects flag, so for testing you will need to run dune build --profile=release for now.

Questions

1. The implementations both use Brr which I find a lot easier to use, but if we want less dependencies etc. I'm happy enough to refactor that out if we want to?
2. We could also perhaps provide an Eio_js module in the same vein as Eio_main, although it would be restricted to only the subset of things the browser can support so I'm not sure how useful that would be?
3. Testing JS code is a little tricky -- for node we could probably do an enabled_if and check for a sufficiently new enough node binary and run it. For the browser, there's not a whole lot we can do beyond manually checking (we could perhaps extend the Alcotest tests and use that e.g. https://github.com/patricoferris/irmin-browser-tests)

[talex5]

This looks very cool!

The implementations both use Brr which I find a lot easier to use, but if we want less dependencies etc. I'm happy enough to refactor that out if we want to?

According to opam list --required-by=brr --recursive, brr actually has fewer dependencies than js_of_ocaml (though both depend on js_of_ocaml-compiler). Though I see you depend on js_of_ocaml too. Maybe we can remove that instead (it pulls in Lwt, for example)?

We could also perhaps provide an Eio_js module in the same vein as Eio_main, although it would be restricted to only the subset of things the browser can support so I'm not sure how useful that would be?

Eio_main is useful because you often want to write a program and not care whether it will run on Linux, macos, etc. Eio_js would only be useful if people are likely to want to write applications that could be run either in the browser or under node, which seems doubtful to me.

Testing JS code is a little tricky

I don't do much web stuff, so no idea.

[talex5]

Looks like this is asking the browser to run schedule again soon, unless we already asked it to do that.

I'd expect to see this code in enqueue_thread, when we have something to do, rather than in schedule when we're idle. Does the current code just busy-wait until there's something to do?

[patricoferris]

That's the idea yeah, afaict using set_timeout with ~ms:0 allows us to yield to underlying JS event loop. So if the timeout is already set schedule effectively exits but our main promise below is not yet fulfilled so we don't get passed that point, the event loop is given a chance to run any events, timeouts etc. and then our schedule gets fired and we check to see if anything is ready.

But your question made me rethink it and I think there's a nicer solution where the scheduler becomes an event listener, see this change patricoferris@935b2e8

[patricoferris]

I've updated this PR to be only the browser backend so hopefully it is easier to review. The scheduler also now uses the event based approach I described above.

W.r.t testing -- I've added some Alcotests that compile to the browser and then render to the DOM. This pulls in Alcotest and Ansi (for nice colours) for testing which is maybe a bit much, especially considering we can't really hook it up to CI.

The tests are all currently passing I thought I would just show an example.

[talex5]

This is looking pretty good (comments inline). CI is failing, though.

[talex5]

Why is this optional?

[patricoferris]

Yep, I implemented it in some weird back to front way that meant there was an interdependency between the schedule function and the scheduler. This is now fixed. Note as well we don't need to track the pending_io anymore either.

[talex5]

This looks wrong. If I understand correctly, cancellation will go like this:

1. Someone creates a Fut and awaits it.
2. The await is cancelled. cancelled is set to true and the waiter is resumed.
3. The fiber starts a new operation.
4. Fut is later resolved. It (incorrectly) clears the cancel function of the fiber's new operation.

Fixing that just requires putting the clear inside the if.

It's also a bit of a problem that we leak memory every time we await and then cancel (until the Fut is finally resolved). Eio generally tries to avoid doing that, but it may be impossible with the Fut API.

[patricoferris]

Yep quite right! I've added a comment about the memory leak. I'm not sure there's a way around this because Fut is based on JS promises and they are eager so don't have a way to cancel them iiuc.

[talex5]

This one never clears the cancel function, so cancelling after it returns (or after it is enqueued) may try to resume twice.

[patricoferris]

Yep, should be fixed now. Also removed the need for the listener ref.

[talex5]

Why doesn't this need to call schedule again after f is done?

[patricoferris]

Yes, good question!

So after a bit of head scratching, I think the answer is it should call schedule again but because of how everything is implemented it doesn't actually need too... maybe. So I think this is because the only time we could potentially hang is if there is more than one item in the run_q and there is no scheduled wakeup call. I think the only way to get more than one thing in the queue is if you do some "IO" like the timeout (so not by forking, awaiting promises etc.) and because every single "IO" operation sends a wakeup event to our scheduler then the run_q will always be fully processed (but perhaps more slowly). At any rate, I've added a call to schedule here!

[talex5]

We need to be careful here. In Eio_linux, schedule returns [`Exit_scheduler] and a suspended fiber has type ('a, [`Exit_scheduler]) continuation. This indicates that resuming a continuation commits to keeping things going (i.e. calling schedule again).

Your run_q has type (unit -> unit) Run_queue.t, which suggests it doesn't do that.

It would probably be best to decide which one you're doing and update the types to check it. If you call schedule when you don't need to, you can easily make your continuation handler not be tail-recursive, which is bad.

[patricoferris]

Good point! So in 9ff3108 I've changed the scheduler again (sorry...). It is now much more reminiscent of the luv scheduler modulo Luv.Async.send is now a call to requestIdleCallback the idea being anytime we fall through to the final "exit" promise we'll be idle and allow the scheduler to be woken up. This means schedule (now wakeup) is only ever called when needed. Unfortunately safari doesn't implement requestIdleCallback but this seems like a shim people have used in their libraries. I do note that react.js used to use it in their scheduler but I think decided that it wasn't aggressive enough, I don't know if that's a problem or not here.

[patricoferris]

I think this may suffer from the same problem as #427

[talex5]

At the time of writing, there seems to be a few issues with dune/js_of_ocaml which I think are being fixed w.r.t to separate compilation.

Is this fixed with dune 3.7.0?

Looks like most tests are now passing, but there's a problem with the lower-bounds.

[patricoferris]

Is this fixed with dune 3.7.0?

Yep! Is it okay to make everything use 3.7 ? In which I case I'll open that separately as it forces a few other changes

[patricoferris]

Just to be clear (the message might have been lost above), the current implementation suffers the same problem as Eio_luv with performing effects across a C call (a JS call in this case). So the following fails:

module Echo = struct
    type _ Effect.t += Echo : string -> unit Effect.t
  
    let run f =
      Effect.Deep.try_with f ()
        {
          effc =
            (fun (type b) (eff : b Effect.t) ->
              match eff with
              | Echo string ->
                  Some
                    (fun (k : (b, unit) Effect.Deep.continuation) ->
                      print_endline string;
                      Effect.Deep.continue k ())
              | _ -> None);
        }
  end
  
  let () =
    Echo.run @@ fun () ->
    let p = 
      Eio_browser.run @@ fun _ ->
      Eio.Fiber.yield ();
      Effect.perform (Echo.Echo "world")
    in
    Fut.await p (fun () -> ())

I was wondering if we could turn the scheduler into a generator function or something, but haven't had the time to look into it or I don't know if that would fix things either.

[talex5]

I think the scheduling needs more documentation. I couldn't convince myself it was correct. I pushed a commit here that adds some types to try to clarify things:

https://github.com/talex5/eio/commits/js

There, I set the continuation return type to suspend, which indicates that the scheduler found the run queue empty and so decided to suspend itself and wait for a callback.

I think the wakeup function might get called in some cases when it's already running (or going to get called).

Also, I think all places where you use enter_unchecked you do actually need to check that the fiber isn't already cancelled before starting.

[patricoferris]

I think the scheduling needs more documentation. I couldn't convince myself it was correct.

Definitely, I could also not convince myself of the correctness either ^^" I've tried a slightly different approach in patricoferris@bf3f701 which I need to document but I think is already simpler and similar to some of the ideas in Eio_luv. Thought I'm not sure of its tail-recursiveness but it might be. I need to go over it properly but it actually also passes the busy yielding test I added too. I'll try and document it properly this week some time.

[patricoferris]

It also dawned on me a little whilst testing this code that perhaps this really doesn't need to be in Eio at all since the JS browser backend is unlikely to ever do any IO (at least not in the Eio sense). The closest things are probably the Fetch API and the Websocket API, but even then Eio's datagrams don't fit with Websocket ones. There's a good argument to having an Eio_node backend where all that IO is possible again (I think ocamllsp could use that for example).

Maybe this is a good example of why having Eio.core separated (the effects and some of the primitives like Promises) into its own library would be a good idea? Then this backend can live elsewhere and not pretend to be an Eio backend?

[talex5]

Maybe this is a good example of why having Eio.core separated (the effects and some of the primitives like Promises) into its own library would be a good idea?

It is its own library (eio.core) already (but not its own package), though it's considered a private API (and is mostly re-exported by eio).

Then this backend can live elsewhere and not pretend to be an Eio backend?

It can already live elsewhere if it wants to. The only advantage of having it in this repository is making sure we keep it up-to-date if we change the core API. Making it separate may well be a good idea.

The eio package itself has several parts (as you know):

• The core: Fiber, Switch and Cancel (for users), and the modules they require (Promise, Cells, Broadcast, etc), plus some modules for backends (Fiber_context, Suspend, etc).
• Types for OS interfaces (flows, networks, filesystems, clocks).
• Additional synchronisation APIs (Semaphore, Stream, Condition).
• Buffered IO (Buf_read and Buf_write).
• The eio.utils library provides some modules that may be useful for implementing backends (e.g. Lf_queue and Zzz).
• eio.unix provides extra APIs for Unix-type operating systems.

In theory, you only need Fiber_context and the three effects (Suspend, Fork and Get_context) to make a backend.

But really you'll end up wanting more. Users of a backend need Fiber and Cancel, with their dependencies Switch and Promise, at least, and will likely want the other synchronisation primitives and buffered reading and writing, so there's little point in splitting those out.

That just leaves the OS types (Net, Fs, Process, etc), but they're mostly just interfaces anyway. I don't think there's much to be gained from splitting them out, especially if you include Flow (which the buffered reader needs, and would therefore depend on).

It might be useful to split the core out to convince people it's modular, though (that's why it's a separate library already). The main problem with splitting out eio.core is that it's a bit arbitrary which things are in it. e.g. Promise is included because Fiber.fork_promise needs it (and it's used internally a bit). If we added a Fiber.fork_stream, we'd want to move Stream in there too, etc, which would become an API change.

but even then Eio's datagrams don't fit with Websocket ones

Is that something we should fix?

[aryx]

Do we have an ETA for this being merged?
I'm strongly considering switching from lwt to eio for some of the code we have at Semgrep, but we need to have the jsoo eio working. We're currently relying on cohttp-lwt-jsoo and so we need a similar cohttp-eio-jsoo.

[balat]

Do we have an ETA for this being merged?

We will resume this work in the next few weeks.
There are still some problems to fix before merging.

[patricoferris]

Anything I can do to help here?

[Sudha247]

I believe @vouillon has started looking at this.

[talex5]

Did we decide to put this in a separate repository? Setting that up might be useful, but I don't want to conflict with anything that @vouillon is doing.

[patricoferris]

In the short-term I've made a separate package https://github.com/patricoferris/eio_browser -- I'm happy to maintain and release this, but should probably go into ocaml-multicore ?

[talex5]

Sounds good. Do you have permission to transfer it? If not maybe @Sudha247 can add it (or you could transfer it to me and I can do it).

[patricoferris]

@talex5 perfect, transfer requested to you :))

[talex5]

Done: https://github.com/ocaml-multicore/eio_browser

However, I no longer have admin rights on it, so can't give you access! I think @Sudha247 should be able to do it.

[talex5]

I guess we should move #680 to the new repository too?

[patricoferris]

Yep I think that makes sense. @balat Do you want to open your new PR against that repository which perhaps we could rename to eio_js ?

[talex5]

(note: #680 is a new PR by js_of_ocaml maintainer @vouillon; you might have been thinking of #534, which we can probably close now)

[vouillon]

I'm not sure it really makes sense to move #680 to the new repository, since there is not much in common in the end. i should probably set up a different repository instead.

[talex5]

@patricoferris what do you think about replacing the current eio_browser repository with #680? Is there any reason to have both?

[vouillon]

@patricoferris what do you think about replacing the current eio_browser repository with #680? Is there any reason to have both?

@talex5 I have also created a repository vouillon/eio_js which could be moved to ocaml-multicore. I'm currently keeping it in sync with #680. I would be interested in feedback from both of you.

[patricoferris]

Yes, apologies, I did mean open #680 to ocaml-multicore/eio_browser to replace it and if we need to rename it to eio_js then let's do that. Probably it is easier to mport vouillon/eio_js into this org and delete eio_browser entirely

[patricoferris]

Either way, this PR is no longer relevant here.