Skip to content

Commit

Permalink
fix native_thread_destroy() timing
Browse files Browse the repository at this point in the history
With M:N thread scheduler, the native thread (NT) related resources
should be freed when the NT is no longer needed. So the calling
`native_thread_destroy()` at the end of `is will be freed when
`thread_cleanup_func()` (at the end of Ruby thread) is not correct
timing. Call it when the corresponding Ruby thread is collected.
  • Loading branch information
ko1 committed Oct 12, 2023
1 parent 10ba3fc commit 0d93441
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 24 deletions.
5 changes: 0 additions & 5 deletions thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -508,16 +508,11 @@ thread_cleanup_func(void *th_ptr, int atfork)
* Unfortunately, we can't release native threading resource at fork
* because libc may have unstable locking state therefore touching
* a threading resource may cause a deadlock.
*
* FIXME: Skipping native_mutex_destroy(pthread_mutex_destroy) is safe
* with NPTL, but native_thread_destroy calls pthread_cond_destroy
* which calls free(3), so there is a small memory leak atfork, here.
*/
if (atfork)
return;

rb_native_mutex_destroy(&th->interrupt_lock);
native_thread_destroy(th);
}

static VALUE rb_threadptr_raise(rb_thread_t *, int, VALUE *);
Expand Down
5 changes: 0 additions & 5 deletions thread_none.c
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,6 @@ ruby_mn_threads_params(void)
{
}

static void
native_thread_destroy(rb_thread_t *th)
{
}

void
ruby_init_stack(volatile VALUE *addr)
{
Expand Down
25 changes: 11 additions & 14 deletions thread_pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -1714,14 +1714,17 @@ native_thread_assign(struct rb_native_thread *nt, rb_thread_t *th)
}

static void
native_thread_destroy(rb_thread_t *th)
native_thread_destroy(struct rb_native_thread *nt)
{
struct rb_native_thread *nt = th->nt;

rb_native_cond_destroy(&nt->cond.readyq);

if (&nt->cond.readyq != &nt->cond.intr)
rb_native_cond_destroy(&nt->cond.intr);
if (&nt->cond.readyq != &nt->cond.intr) {
rb_native_cond_destroy(&nt->cond.intr);
}

RB_ALTSTACK_FREE(nt->altstack);
ruby_xfree(nt->nt_context);
ruby_xfree(nt);
}

#if defined HAVE_PTHREAD_GETATTR_NP || defined HAVE_PTHREAD_ATTR_GET_NP
Expand Down Expand Up @@ -2262,10 +2265,9 @@ rb_threadptr_sched_free(rb_thread_t *th)
{
#if USE_MN_THREADS
if (th->sched.malloc_stack) {
// has dedicated
ruby_xfree(th->sched.context_stack);
RB_ALTSTACK_FREE(th->nt->altstack);
ruby_xfree(th->nt->nt_context);
ruby_xfree(th->nt);
native_thread_destroy(th->nt);
}
else {
nt_free_stack(th->sched.context_stack);
Expand All @@ -2280,12 +2282,7 @@ rb_threadptr_sched_free(rb_thread_t *th)
th->nt = NULL;
#else
ruby_xfree(th->sched.context_stack);

struct rb_native_thread *nt = th->nt;
if (nt) { // TODO: not sure why nt is NULL
RB_ALTSTACK_FREE(nt->altstack);
ruby_xfree(nt);
}
native_thread_destroy(th->nt);
#endif
}

Expand Down
1 change: 1 addition & 0 deletions thread_win32.c
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ th_has_dedicated_nt(const rb_thread_t *th)
void
rb_threadptr_sched_free(rb_thread_t *th)
{
native_thread_destroy(th->nt);
ruby_xfree(th->nt);
ruby_xfree(th->sched.vm_stack);
}
Expand Down

0 comments on commit 0d93441

Please sign in to comment.