Check_foo functions are not mpsafe due to concurrent Format access

[haesbaert]

If you run check tests in multiple domains things will crash. Alcotest uses Fmt, which in turn uses a Queue somewhere that is manipulated at each check_foo call, since this Queue is not protected, it ends up getting corrupted and we get a Queue.Empty exception.

Just running two Domains in parallel and a check_int "foo" 0 0 in a loop is enough to reproduce.

I'll investigate better in the afternoon and maybe come up with a fix, I'm ccing @dbuenzli as he might be interested in making Fmt mpsafe, if not we'll fix it here only.

tldr: We can't really use alcotest for multicore tests reliably until we sort this.

[dbuenzli]

As far as I know:

1. no Queue is being used by Fmt
2. Formatters are not thread safe anyways.

So I'm not sure what kind of "fix" Fmt should implement.

[haesbaert]

I might be off, leme isolate this better before bothering you again.

…

On Wed, Mar 8, 2023, 10:19 Daniel Bünzli ***@***.***> wrote: As far as I know: 1. no Queue is being used by Fmt 2. Formatters are not thread safe anyways. So I'm not sure what kind of "fix" Fmt should implement. — Reply to this email directly, view it on GitHub <#375 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABR2EA5O2ZBPECENMEYH5TW3BFKVANCNFSM6AAAAAAVTQAPZI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Octachron]

Format itself does use a queue internally: a formatter should be owned by a single domain which suggest that each test should output to its own distinct formatter if you want them to run in parallel.

[haesbaert]

Indeed, I now had time to get a proper stack trace, apologies for hinting at Fmt, here it is:

Stdlib.Queue.Empty
Raised at Stdlib__Queue.take in file "queue.ml", line 73, characters 11-22
Called from Stdlib__Format.advance_left in file "format.ml", line 436, characters 6-31
Called from Stdlib__Format.pp_flush_queue in file "format.ml", line 613, characters 2-20
Called from Stdlib__Format.pp_print_flush in file "format.ml", line 673, characters 2-28
Called from Fmt.flush in file "src/fmt.ml" (inlined), line 35, characters 18-46
Called from Alcotest_engine__Test.show_assert in file "src/alcotest-engine/test.ml", line 154, characters 6-27
Called from Alcotest_engine__Test.check in file "src/alcotest-engine/test.ml", line 180, characters 2-17
Called from Dune__exe__Test.check_int in file "test/test.ml" (inlined), line 10, characters 26-35
Called from Dune__exe__Test.T.alcotest_multicore.loop in file "test/test.ml", line 372, characters 6-21
Called from Dune__exe__Test.T.alcotest_multicore in file "test/test.ml", line 377, characters 4-15
Called from Alcotest_engine__Core.Make.protect_test.(fun) in file "src/alcotest-engine/core.ml", line 180, characters 17-23
Called from Alcotest_engine__Monad.Identity.catch in file "src/alcotest-engine/monad.ml", line 24, characters 31-35

reproduces with:

  let alcotest_multicore () =
    let rec loop c () =
      check_int c 0 0;
      Domain.cpu_relax ();
      loop c ()
    in
    let b = Domain.spawn (loop "b") in
    loop "a" () |> ignore;
    Domain.join b

[haesbaert]

Format itself does use a queue internally: a formatter should be owned by a single domain which suggest that each test should output to its own distinct formatter if you want them to run in parallel.

I'll see who to fix this in alcotest, I imagine we could have a DLS for it and should be good enough.

[haesbaert]

Problem appears to be this:
Alcotest uses Fmt.stdout which is Format.std_formatter which in turn is created via make_formatter -> pp_make_formatter, which is not mpsafe.
Format has a synchronized_formatter_of_out_channel, which is domain safe. Maybe alcotest should create its own safe stdout_formatter and use that all over.

[dinosaure]

Fixed by #399, It is possible now to specify at the beginning a new allocated formatter that tests don't use - and by this way, fix a race between default formatters (like Fmt.stdout) and user specified ones.