search

mirage / alcotest

A lightweight and colourful test framework
ISC License
456 stars 80 forks source link

Proof of concept - Fix the race with Formatters (issue #375) #376

Closed haesbaert closed 6 months ago

haesbaert commented 1 year ago

I have no idea where these should be, but this is enough to fix the race, if someone could step in I'd appreciate.

haesbaert commented 1 year ago

issue #375

samoht commented 1 year ago

The fix looks ok to me, just two questions:

Sudha247 commented 1 year ago

what's the cost of looking for something in DLS for every print operation?

Lookup on DLS is constant time.

is there a better way to use both the OCaml 5 API and OCaml 4? For instance, could we have a mock version for DLS somewhere?

We have an OCaml 4 compatible Domain API in domain-shims, it also supports DLS -- https://gitlab.com/gasche/domain-shims/-/blob/trunk/lib/domain.ocaml4.mli#L88. The point to note here is domain-shims uses systhreads internally to simulate domains. If that's an issue, a mock DLS implementation for OCaml 4 should be fairly straightforward.

haesbaert commented 1 year ago

The fix looks ok to me, just two questions:

  • what's the cost of looking for something in DLS for every print operation? (I suspect this is not really an issue as people are not expected to write to stdout/stderr in a hot loop, but I'm still interested to know the answer :p)

Not that I profiled, but I'd say that it's many magnitudes less than the actual IO operation.

  • is there a better way to use both the OCaml 5 API and OCaml 4? For instance, could we have a mock version for DLS somewhere?

Even if we mock the DLS we would still need to mock the Format.synchronized_formatter_of_out_channel.

A simpler alternative could be to convert all prints to some function that grabs a mutex, and then we just use the normal output channel, this way the code could be the same for 4 and 5.

samoht commented 1 year ago

@haesbaert do you mind adding a change entry? I'm happy to merge/release this quickly to make a release compatible with multi-domains.

haesbaert commented 1 year ago

@haesbaert do you mind adding a change entry? I'm happy to merge/release this quickly to make a release compatible with multi-domains.

I've reworded the commit and added the Changes entry.

dinosaure commented 6 months ago

Close by #399