fix(rpc): remove threads

[rgrinberg]

replace them with evented io based on Unix.select

Signed-off-by: Rudi Grinberg me@rgrinberg.com

[rgrinberg]

ping @nojb

[nojb]

I will try to review this, but I have some general questions:

• What is the motivation for this change? In other words, what is the main expected benefit of the changes in this PR with respect to the existing code?
• The doc mentions that pipes are used on Unix. They are not used on Windows? Is something else used instead?

[rgrinberg]

There's a couple of benefits:

1. We stop leaking memory on every connected client because we no longer spawn threads we don't cleanup.
2. We no longer spawn 3 threads for every connected clients. Threads are heavy and we don't want to have too many running at once. The background threads are better saved for offloading work that can be sped up or that does background file IO.

The doc mentions that pipes are used on Unix. They are not used on Windows? Is something else used instead?

Where does it mention that? I think we switched to sockets everywhere to simplify the implementation.

[nojb]

Where does it mention that? I think we switched to sockets everywhere to simplify the implementation.

Here:

https://github.com/ocaml/dune/pull/7418/files#diff-e846a205c1d2ea5350a77593df11a40c52bea09ea62b8cbf33200c4e4e23917aR1

and here:

https://github.com/ocaml/dune/pull/7418/files#diff-45f34c5c2485bce0c8e8b59794034f972dad8b9b34c0d3dc64333b51c2e672ecR190-R193

[rgrinberg]

Oh yeah that's a single pipe I use to be able to interrupt the select. I'm using non-blocking pipes only on Unix - that's what I meant.

[rgrinberg]

Some additional notes:

1. the async reading of csexp is mostly ripped off from my previous implementation in ocamllsp. it should be mostly bug free.
2. We should be careful to make sure we never close an fd that is being selected on.
3. We should make sure there no fd reuse bugs. E.g. two threads should never race to close a single fd as the the thread to lose this race might close some unrelated fd.

[rgrinberg]

A few remarks after glancing at the code. Probably not useful, but just in case:

• I'm not sure why wait should fail if the queue has been shutdown. Seems like it should just return an empty list of events? Also, how does the thread that calls wait in a loop know how to terminate? Shouldn't it check some shutdown flag?

• fsenv->thread is being initialized in the main thread, while wait is being called in another thread. Why does shutdown wait for the main thread to the terminate? Shouldn't it be waiting for the wait thread?

• I would personally prefer if Fswatch_win.shutdown was idempotent. This is consistent with how we define such functions elsewhere in dune (when possible).

[nojb]

A few remarks after glancing at the code.

Thanks, all good points. Not sure about the second point (I have forgotten some of the details); I'll take a closer look and report back. And I'll prepare a PR with the fixes.

[nojb]

Argh, while investigating the Windows issue I realized that the bad behaviour is already present in main (and 3.8.0, but not 3.7.1), so in principle it should not be related to this PR. Do you see any PR merged between 3.7.1 and 3.8.0 that may have changed the behaviour with respect to threading, signals or the like?

[rgrinberg]

You can try bisecting. I can't think of anything that is Windows specific.

By the way, I discovered some issues with this PR and I've updated it to fix them. Nothing is windows specific, but perhaps windows fails in some special way. So perhaps it's worth trying this PR again.

[nojb]

You can try bisecting. I can't think of anything that is Windows specific.

The culprit seems to be #6996 cc @Alizter

[rgrinberg]

Hmm, that's quite weird. I would assume tui does nothing if it's not enabled.

[Alizter]

There were a few changes for tui (in a different PR) to improve how signals were handled. We'll have to see what went wrong. I have a windows vm so I can also take a closer look.

[nojb]

There were a few changes for tui (in a different PR) to improve how signals were handled. We'll have to see what went wrong. I have a windows vm so I can also take a closer look.

It seems to be related to the replacement of

Dune_threaded_console.progress ()

by

Dune_console.Backend.progress

see
https://github.com/ocaml/dune/pull/6996/files#diff-3b5679e0e3859b1dc8498f8d8b27a51fafc2265b72bf2fa4593d944ee950b634L37

[rgrinberg]

It seems to be related to the replacement of

For a quick fix, how about we go back to the unthreaded console for windows. Should be quite easy.

[nojb]

It seems to be related to the replacement of

For a quick fix, how about we go back to the unthreaded console for windows. Should be quite easy.

I don't understand: the current code uses Dune_console.Backend.progress; the old code used Dune_threaded_console.progress. Does it mean that we currently used the "unthreaded" console and before we used to use the "threaded" one?

[rgrinberg]

Does it mean that we currently used the "unthreaded" console and before we used to use the "threaded" one?

I think I confused myself with all these UI switches we've had :)

Looking at the current state, I see that our progress reporting does not rely on an additional thread. I don't know why, but my intention was that it would.

From the diff you've pointed out, switching from threaded to unthreaded introduced an issue. I have no idea why, but it's worth it to toggle it back to threaded to see if you can get the bug to go away.

[nojb]

From the diff you've pointed out, switching from threaded to unthreaded introduced an issue. I have no idea why, but it's worth it to toggle it back to threaded to see if you can get the bug to go away.

I tried, but then the progress line completely disappeared, and the system did not appear very responsive... Not sure if it is an issue on Windows only or if it affects Linux as well; I'll try to investigate more later.

[rgrinberg]

In any case, I think we're convinced this PR doesn't introduce this problem. Since it also fixes a serious issue with RPC, I'm merging.