Free everything at exit #8868
Free everything at exit #8868
Conversation
|
@HParker Please add |
15d6556 to
8435b9c
Compare
|
oh! I misunderstood the failure and added |
|
Actually @mame, am I able to use |
8435b9c to
998ed24
Compare
|
@HParker Sorry, if they need to refer across files, it is OK to add |
998ed24 to
2db85c4
Compare
internal/vm.h
Outdated
| void rb_free_global_enc_table(void); | ||
| // rb_free_loaded_builtin_table needs a week definition in case minibuiltin.c is not used | ||
| // this is required for the wasm build. | ||
| void rb_free_loaded_builtin_table() __attribute__((weak)); |
There was a problem hiding this comment.
Looks like __attribute__((weak)) is not supported in Windows, but removing this break the wasm build.
Maybe there is another builtin.c equivalent place I need to define this so that wasm has it and Windows is happy?
There was a problem hiding this comment.
That's all I can think of. I haven't tried but maybe:
diff --git a/builtin.c b/builtin.c
index aef5b2c2d4..c0cd0f6e50 100644
--- a/builtin.c
+++ b/builtin.c
@@ -57,6 +57,12 @@ rb_load_with_builtin_functions(const char *feature_name, const struct rb_builtin
#endif
+void
+rb_free_loaded_builtin_table(void)
+{
+ // do nothing
+}
+
void
Init_builtin(void)
{
diff --git a/internal/vm.h b/internal/vm.h
index 059c106e44..94434048da 100644
--- a/internal/vm.h
+++ b/internal/vm.h
@@ -91,7 +91,7 @@ void rb_free_environ(void);
void rb_free_global_enc_table(void);
// rb_free_loaded_builtin_table needs a week definition in case minibuiltin.c is not used
// this is required for the wasm build.
-void rb_free_loaded_builtin_table() __attribute__((weak));
+void rb_free_loaded_builtin_table();
void rb_free_rb_global_tbl(void);
void rb_free_shared_fiber_pool(void);
void rb_free_static_symid_str(void);
diff --git a/mini_builtin.c b/mini_builtin.c
index 4926afd031..a0bb46c54c 100644
--- a/mini_builtin.c
+++ b/mini_builtin.c
@@ -10,13 +10,6 @@
#ifndef INCLUDED_BY_BUILTIN_C
static struct st_table *loaded_builtin_table;
-
-void
-rb_free_loaded_builtin_table(void)
-{
- if (loaded_builtin_table)
- st_free_table(loaded_builtin_table);
-}
#endif
rb_ast_t *rb_builtin_ast(const char *feature_name, VALUE *name_str);
diff --git a/miniinit.c b/miniinit.c
index 2a14a0d1c5..bb22d6d9f5 100644
--- a/miniinit.c
+++ b/miniinit.c
@@ -49,3 +49,10 @@ Init_enc(void)
}
#include "mini_builtin.c"
+
+void
+rb_free_loaded_builtin_table(void)
+{
+ if (loaded_builtin_table)
+ st_free_table(loaded_builtin_table);
+}There was a problem hiding this comment.
@mame It seems fine.
Just one thing, don't use pre-ANSI-style declaration.
7a3c312 to
11cee78
Compare
vm.c
Outdated
| rb_objspace_free_objects(objspace); | ||
| rb_free_generic_iv_tbl_(); | ||
| if (th) { | ||
| xfree(nt); |
There was a problem hiding this comment.
on Mac, this fails with,
pointer being freed was not allocated
but I am confused since it seems like,
vm->ractor.main_thread
is always ZALLOCed in Init_BareVM.
9c642a9 to
7b8d4ee
Compare
7b8d4ee to
6b8df8e
Compare
8c3eb0f to
bbded12
Compare
|
I tested this branch with a Rails app, and I get a double free from freeing environ, I am going to stop freeing environ which means it will show up as a leak, but it allows testing with less trivial Ruby scripts. I would love a better solution, but I am not sure how to determine when/if something in environ is "owned" by the vm. |
bbded12 to
3f3f4b3
Compare
|
I would've liked to see |
|
I regret saying |
3f3f4b3 to
83700be
Compare
|
I looked around the code and found more examples of `getenv("RUBY_*" so I chose that as the prefix. Happy to change it, but that seems like the most consistent name. Thanks for the suggestion @Edwing123 |
| st_free_table(vm->static_ext_inits); | ||
| st_free_table(vm->ensure_rollback_table); | ||
|
|
||
| ruby_xfree(vm->postponed_job_buffer); |
There was a problem hiding this comment.
While working on another issue with the postponed job infrastructure, I realised there's something a little bit annoying about doing this... It's the most miniscule of corner cases, but I figure I'll bring it up.
If some other gem has registered a signal handler, it's possible a signal might arrive after this and before the process exits, the gem's signal signal handler could then try and call rb_register_postponed_job, which will be a use-after-free here.
Stackprof in particular tries to get this right, and almost does: it registers a ruby_vm_at_exit hook here: https://github.com/tmm1/stackprof/blob/91d12e77b3d3c5014870b4ee7c19eab24ad04b5b/ext/stackprof/stackprof.c#L903 to disable itself.
There are just a couple teeny tiny problems though:
The first problem is, that the free-on-shutdown stuff is happening before ruby_vm_run_at_exit_hooks. Easy fix, let's hoist that further up I suppose? Or push this stuff further down?
The second problem is what happens in the presence of non-ruby-managed threads. If there are other native threads in the process, a signal handler could be in-progress on that thread already when ruby_vm_run_at_exit_hooks is called, so setting ruby_vm_running = 0; wouldn't stop it from happening.
I actually can't think of anything at all we can do about that though. If there was a way to run something at exit after all other threads have been destroyed, we could do that, but there isn't such a thing. I guess well-behaved extensions need to make sure they pthread_join all their threads in a ruby_vm_at_exit handler?
when the RUBY_FREE_ON_SHUTDOWN environment variable is set, manually free memory at shutdown. Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org> Co-authored-by: Peter Zhu <peter@peterzhu.ca>
83700be to
16acb7c
Compare
|
@HParker @peterzhu2118 @byroot Matz suggested |
For more information, see: - https://bugs.ruby-lang.org/issues/19993 - ruby/ruby#8868 - ruby/ruby#9302
when the FREE_ON_SHUTDOWN environment variable is enabled, manually free memory at shutdown.
https://bugs.ruby-lang.org/issues/19993
without
FREE_ON_SHUTDOWN:with
FREE_ON_SHUTDOWN=1