mirage / ocaml-conduit

Dereference URIs into communication channels for Async or Lwt
ISC License
84 stars 74 forks source link

conduit-lwt-unix no longer calls Mirage_crypto_rng_unix.initialize #318

Closed xguerin closed 4 years ago

xguerin commented 4 years ago

Since updating to 2.2.2 my web app based on opium fails in SSL mode with the following error:

[WARNING] Uncaught exception in handler: The default generator is not yet initialized.
...
  If you are using Lwt, execute `Mirage_crypto_rng_lwt.initialize ()` at the beginning of your event loop (`Lwt_main.run`) execution.

I just saw your CHANGES.md and have a question: whose responsibility should it be to call that Mirage_crypto_rng_lwt.initialize () function now that conduit_lwt does not call it anymore?

Thanks,

avsm commented 4 years ago

Oh dear, this is a dynamic failure indeed. I might have merged that into opam-repo too soon.

@hannesm, we could propose a PR to Lwt that also adds enter_hooks alongside the existing iter hooks and exit hooks. Would that remove the need for every cohttp user to add this to their Lwt main?

hannesm commented 4 years ago

propose a PR to Lwt that also adds enter_hooks

@avsm that is a possibility.

I might have merged that into opam-repo too soon.

for a sound entropy experience, I don't see an option right now how to initialize the rng in a library.

@xguerin the responsibility is that the application should call Mirage_crypto_rng_lwt.initialize () in Lwt_main.run. This is nothing a library can do (and using Mirage_crypto_rng_unix.initialize () -- as previously done by conduit -- does not give the same result, i.e. no entropy collection).

hannesm commented 4 years ago

sorry, had a mouse issue and closed prematurely.

xguerin commented 4 years ago

Thanks for the clarification.

avsm commented 4 years ago

As a workaround for now, we could use the Lwt_main enter iter hooks to do something on the lines of

let register_hook () =
  let crypto_init = ref false in
  let enter_hook () =
    if not !crypto_init then
      Mirage_crypto_rng_unix.initialize ()
  in
  Lwt_main.Enter_iter_hooks.add_first enter_hook

Although, this is roughly what mirage-crypto-rng-lwt is already doing in: https://github.com/mirage/mirage-crypto/blob/master/rng/lwt/mirage_crypto_rng_lwt.ml#L58

Why does it need to be run within Lwt_main? Is it because multiple invocations of it warn, and so it's bad for a library to do this?

hannesm commented 4 years ago

Why does it need to be run within Lwt_main?

ah, you're right - a solution may be to move Mirage_crypto_rng_lwt.initialize out of the Lwt monad, and call it from toplevel in conduit-lwt-unix. How does Lwt.async behave when not executed within the Lwt monad?

Is it because multiple invocations of it warn, and so it's bad for a library to do this?

it is best to have only one entropy collection periodic task running for a single default rng.

avsm commented 4 years ago

Right, I definitely agree that just one entropy collection task is appropriate. However, calling the initialiser multiple times could be idempotent. The reason is that we can stash the enter hook that returns from Lwt_main.Enter_iter_hooks and not take any action if one is already registered.

This way a toplevel initialiser in conduit-lwt-unix would be safe. Otherwise its not, as multiple libraries trying to initialise would result in link time warnings. Lwt.async outside the main loop looks fine -- in general in Lwt, everything just runs to "blocking" and is then woken up by the main loop, so the only unsafe thing is to nest Lwt_main.run calls.

hannesm commented 4 years ago

as update, conduit-lwt-unix 2.2.2 was disabled (in https://github.com/ocaml/opam-repository/pull/16648).

I came up with the following snippet:

open Lwt.Infix

let () =
  Lwt.async (fun () ->
      let rec go i =
        Printf.printf "async %d\n%!" i;
        Lwt_unix.sleep 1. >>= fun () ->
        go (succ i)
      in
      go 0);
  let foo = ref 0 in
  let _ = Lwt_main.Enter_iter_hooks.add_first
      (fun () -> Printf.printf "entering %d\n%!" !foo; incr foo)
  in
  Printf.printf "now starting main\n%!";
  Lwt_main.run (Lwt_unix.sleep 10.);
  Printf.printf "now main stopped\n%!";
  Unix.sleepf 3.;
  Printf.printf "now starting main again\n%!";
  Lwt_main.run (Lwt_unix.sleep 10.)

which (both native and bytecode) behaves as follows:

async 0
now starting main
entering 0
async 1
entering 1
async 2
entering 2
async 3
entering 3
async 4
entering 4
async 5
entering 5
async 6
entering 6
async 7
entering 7
async 8
entering 8
async 9
entering 9
now main stopped
now starting main again
entering 10
async 10
entering 11
async 11
entering 12
async 12
entering 13
async 13
entering 14
async 14
entering 15
async 15
entering 16
async 16
entering 17
async 17
entering 18
async 18
entering 19
async 19
entering 20

I hope that behaviour is intentional (and guaranteed by lwt). If this is the case, I'd suggest to (a) modify Mirage_crypto_rng_lwt.initialize to not be in the Lwt monad (since it only registers async and Enter_iter_hooks, there's no need to be in the monad) and (b) call that function from top-level in conduit-lwt-unix (will depend on a mirage-crypto-rng release).

Does this sound good to you? //cc @avsm

avsm commented 4 years ago

This sounds great to me @hannesm!

talex5 commented 4 years ago

This will also be helpful with capnp-rpc-unix (it also calls Mirage_crypto_rng_unix.initialize).

hannesm commented 4 years ago

updates: mirage-crypto-rng 0.8 is released where the Mirage_crypto_rng_lwt.initialize () no longer is inside the lwt monad. additionally, https://github.com/ocaml/opam-repository/pull/16682 releases tls 0.12.2 which again (similar to 0.12.0 and before) initializes the rng at the toplevel of tls.lwt. also marks 0.12.1 as unavailable to avoid the subtle changes in semantics.

since conduit-lwt-unix in Conduit_lwt_tls_real uses tls.lwt, we don't need further conduit changes (no need to initialize the rng in here), but can (a) bump the tls conflict to < 0.12.2 (b) mark conduit-lwt-unix 2.2.2 as available in opam repository (once tls 0.12.2 is in). WDYT?

avsm commented 4 years ago

Thanks @hannesm, this is excellent. I'll mark conduit-lwt-unix as available once tls 0.12.2 is merged. @talex5 @xguerin this should then continue to work with opium and capnp-rpc without you having to do any changes. Please let us know if this is not the case.