Fixed memory leak from sigaltstack.

[azewierzejew]

Currently stack for handling signals on the first domain always leaks.
As far as I have read the code extra spawned domains have their stacks cleared on exit.
The following fix clears the stack on the original domain.

It wouldn't free data if caml_shutdown was called on different thread than caml_startup_pooled.
It wouldn't break anything either way (as the function would do nothing).
But I don't know whether calling those functions from different threads is allowed.

[gasche]

As a newcomer to this part of the runtime, I was surprised by this PR because "freeing the signal stack" was already discussed in #10726 and I understood it to be a solved issue. Here is what I understand:

• create_domain mallocs a signal stack (caml_init_signal_stack)
• domain_terminate frees the signal stack (caml_free_signal_stack)
• but the main domain (domain 0) is created with create_domain by caml_init_domains (itself calld by caml_init_gc called by caml_startup_common) but not collected with domain_terminate (right?), so its own signal stack remains unfreed

(Why is this not an issue for other cleanup things that are done in domain_terminate? I see for example the same pattern with CAML_EVENTLOG_{INIT,TEARDOWN}.)

Ideally each init function would have a corresponding free/teardown function, with symmetric code, and the caml_shutdown function would call the free_foo functions for each init_foo function called in caml_startup_*, and nothing more. The proposed patch does something that seems correct but is also abstract-breaking in this sense -- we cleanup directly in caml_shutdown a resource that was allocated deep inside startup chain. It may be nice to have caml_free_domains in domain.c, and caml_free_gc in gc_ctrl.c, and call caml_free_gc from caml_shutdown.

[gasche]

One aspect I wonder about is: shouldn't we call domain_terminate on the main domain as well? Some of its work is meant for synchronization with other running domains and wouldn't be necessary, but that seems fine? Are there other reasons for not doing it?

(cc @dra27 @kayceesrk @ctk21 @Engil)

[azewierzejew]

@gasche Vast majority of other allocation is done with caml_stat_* functions so there is no need to free.
Stack can't be allocated on the pool, because then there will be a dangling pointer when pool is freed.

As for if it is the best solution, I have no idea.

[dra27]

Just as a note, I think everything in #10726 was (intentionally) lost as part of the multicore PR merge, so it's not completely surprising that this might have reappeared. The sigaltstack freeing in 4.x is tied in with 4.x's signal handling, which is completely overhauled in 5.0

[azewierzejew]

Just as a note, I think everything in #10726 was (intentionally) lost as part of the multicore PR merge, so it's not completely surprising that this might have reappeared. The sigaltstack freeing in 4.x is tied in with 4.x's signal handling, which is completely overhauled in 5.0

If I'm correct that trunk of 5.0 is https://github.com/ocaml-multicore/ocaml-multicore then it isn't fixed as there is the complementary problem (freeing stack without reseting).

I've created an issue for that ocaml-multicore/ocaml-multicore#838
so you can choose were it should be discussed.

[dra27]

The ocaml-multicore repository is mostly for historical interest now (earlier versions of multicore, and so forth) - it's just branch trunk on ocaml/ocaml which matters now (5.0 will be branched from it relatively soon).

[azewierzejew]

As a newcomer to this part of the runtime, I was surprised by this PR because "freeing the signal stack" was already discussed in #10726 and I understood it to be a solved issue. Here is what I understand:

* `create_domain` mallocs a signal stack (`caml_init_signal_stack`)

* `domain_terminate` frees the signal stack (`caml_free_signal_stack`)

* but the main domain (domain 0) is created with `create_domain` by `caml_init_domains` (itself calld by `caml_init_gc` called by `caml_startup_common`) but not collected with `domain_terminate` (right?), so its own signal stack remains unfreed

(Why is this not an issue for other cleanup things that are done in domain_terminate? I see for example the same pattern with CAML_EVENTLOG_{INIT,TEARDOWN}.)

Ideally each init function would have a corresponding free/teardown function, with symmetric code, and the caml_shutdown function would call the free_foo functions for each init_foo function called in caml_startup_*, and nothing more. The proposed patch does something that seems correct but is also abstract-breaking in this sense -- we cleanup directly in caml_shutdown a resource that was allocated deep inside startup chain. It may be nice to have caml_free_domains in domain.c, and caml_free_gc in gc_ctrl.c, and call caml_free_gc from caml_shutdown.

I've created a new proposition in https://github.com/azewierzejew/ocaml/tree/signal-stack-fix-alternative
It is more inline with your comment and additionally it unregisters ocaml signal handlers before exiting.
By that I mean it resets all signal handlers to default that were changed.

[gasche]

Your "alternative" PR is sensibly more invasive in terms of change, and I don't feel competent enough to review it by myself. It combines three things:

• it restructures the cleanup functions, I unconditionally like this part
• it calls terminate_domain on the main domain, I think this is right but I need more work to think throug the implications
• it contains signal cleanup code that I have no expertise about and would need another review

I would be in favor of un-drafting the present PR and merging it to fix the leak, and then discussing restructurations as a second step.

[sadiqj]

caml_free_signal_handling on your alternative tree looks fine. I'm not sure I fully understand the need for caml_free_gc as it just seems to redirect to caml_free_domains at the moment. I probably also need to think through the implications of domain_terminate being called during shutdown.

Agree with @gasche , we should un-draft this PR, merge it and then follow up with a restructuring.

[gasche]

I'm not sure I fully understand the need for caml_free_gc as it just seems to redirect to caml_free_domains at the moment.

This mirrors the structure of the initialization code, where caml_init_domains is, somewhat weirdly, called by caml_init_gc. (One justification might be that caml_init_domains also initializes the minor GC for each domain, and maybe caml_init_gc means to initialize all GCs, not just the major GC. I would still find it clearer if the two were separated.)

I probably also need to think through the implications of domain_terminate being called during shutdown.

We're in the same boat, it's not a trivial change, but I have a good feeling about it: I think it's more likely to be right (and remain right as the code evolves) as the current approach -- we should ensure that create_domain is always followed by domain_terminate on successful termination.

[azewierzejew]

I undrafted the PR.

As for using domain_terminate I've tried to read into and noticed that error handling code in create_domain looks kind of leaky.
For example it "leaks" the caml_num_running_domains counter, because on fail the counter is not decreased, but also the domain_terminate isn't called after that as domains_self is set to NULL.

Similar things happens to domain_self->ephe_state. It is allocated in caml_init_major_gc but isn't freed in caml_teardown_major_gc but manually in domain_terminate. So any error past initiation of major gc will leak that (but it would be pooled).

[gasche]

I'm approving the current minimal fix, hoping that we can discuss restructuration of the cleanup code next.

[azewierzejew]

Also calling domain_terminate currently won't work perfectly because there is assertion that backup thread is running, which isn't true on domain 0 if no other domains were spawned (for domain 0 it is spawned lazily in caml_domain_spawn).

[sadiqj]

@azewierzejew if you could do an update of Changes with reviewers, that should also give appveyor a kick and once it's all green I can merge.

[sadiqj]

Thanks @azewierzejew !