Free channel buffers on close rather than leaving them to the GC.

[damiendoligez]

Instead of using finalization to free a channel's buffer, let's free it when the channel is closed. This will save memory (they are freed earlier) and avoid messing up the GC pacing system with the allocation of stdin, stdout, and stderr, which currently accelerate the GC for nothing (their expected lifetime is the lifetime of the program).

This PR also fixes two bugs:

• a variant of A seek on a closed channel can allow invalid inputs #11878 that leads to reading uninitialized memory
• the first write on a closed channel fails as expected, but subsequent writes smaller than the buffer will succeed

fixes #12898

[xavierleroy]

What happens if one thread closes a channel while another one is reading or writing into it?

[damiendoligez]

What happens if one thread closes a channel while another one is reading or writing into it?

In OCaml, same as before: each operation take the channel's lock before doing anything, so one of them will wait until the other is done.

In C, same as before: if you don't lock the channels explicitly, you can shoot yourself in the foot (as specified in io.h).

[jmid]

I've now taken a look at this PR.
When looking at the invariants as part of #12898 I couldn't think of a quick fix myself.
The PR's decoupling of buffer from a channel and using a dummy_buff however allows to keep most of the existing logic untouched.

I also appreciate how the PR documents and cleans up the Channel invariants.

IIUC, the described invariant only holds for channels with CHANNEL_FLAG_MANAGED_BY_GC flag set, right?

Would it make sense to CAMLassert the invariants at selected places to help ensure them going forward?

Testing-wise, I've given the PR a local test run with multicoretests and it held up nicely to the test that initially triggered #12898.

IIUC, the memory leak reported in #12300 should not be reintroduced, because the fix from #12314 is preserved, which will cause the 64k to be reclaimed by the finalizer in such situations where it isn't already freed much earlier on close. It would perhaps still be nice if @edwintorok would give it a spin on their non-trivial application that depends on and exercises these tricky parts...

[gasche]

My gut feeling: this approach is also reasonable, but the use of a one-character buffer is a bit of a hack. (We could be a bit more explicit about the error state, and add a test in flush_partial and refill, those are cold paths.)

[gasche]

Approved -- also on behalf of @jmid.

I think that the code could be clarified a bit, but I like that the current version was heavily tested by @jmid, I think we should consider that it is correct and merge for 5.2.

@Octachron wants to have a look first. Go ahead!

[edwintorok]

It would perhaps still be nice if @edwintorok would give it a spin on their non-trivial application that depends on and exercises these tricky parts...

Thanks, that application is still on 4.14.1 unfortunately. I looked into backporting this to 4.14 (4.14...edwintorok:ocaml:4.14-channels), but the refcounting doesn't quite match 5.x and I'm not convinced my backport attempt is correct, I think I might also need 8198d15, however backporting that is not trivial (and might have other prerequisites).

This change applies cleanly on top of 5.2 though (5.2...edwintorok:ocaml:free-channel-buffers-on-close-5.2), so I'll try to see whether I can test our application with that, but it'll take a while, so don't wait for me: this PR has a well defined small testcase that shows an improvement, if there are more bugs in this area I can open another issue with a testcase.

[gasche]

I rebased on trunk and added a Changes entry in the 5.2 section.