-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force all domain threads to exit before the main thread #13010
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1091,6 +1091,13 @@ static void install_backup_thread (dom_internal* di) | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static void terminate_backup_thread (dom_internal *di) | ||||||||||||||
{ | ||||||||||||||
atomic_store_release(&di->backup_thread_msg, BT_TERMINATE); | ||||||||||||||
/* Wakeup backup thread if it is sleeping */ | ||||||||||||||
caml_plat_signal(&di->domain_cond); | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static void caml_domain_initialize_default(void) | ||||||||||||||
{ | ||||||||||||||
return; | ||||||||||||||
|
@@ -1624,6 +1631,28 @@ void caml_interrupt_all_signal_safe(void) | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static void stw_terminate_domain(caml_domain_state *domain, void *data, | ||||||||||||||
int participating_count, | ||||||||||||||
caml_domain_state **participating) | ||||||||||||||
{ | ||||||||||||||
if (!pthread_equal(pthread_self(), *(pthread_t *)data)) { | ||||||||||||||
/* Domains threads forced to exit here will not have a chance to | ||||||||||||||
run domain_terminate() on their own, so we need to ask the backup | ||||||||||||||
thread to terminate here. */ | ||||||||||||||
terminate_backup_thread(domain_self); | ||||||||||||||
atomic_fetch_add(&caml_num_domains_running, -1); | ||||||||||||||
pthread_exit(0); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, this is dealing with other threads running on the same domain by not releasing the domain lock. Since the other threads cannot acquire the domain lock, they won't access the runtime state. Doesn't the same reasoning apply for the backup thread? As in, you do not need to terminate it? Another potential bug (perhaps unrelated to this PR) is that threads calling |
||||||||||||||
} | ||||||||||||||
dustanddreams marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
void caml_terminate_all_domains(void) | ||||||||||||||
{ | ||||||||||||||
while (!caml_domain_alone()) { | ||||||||||||||
pthread_t myself = pthread_self(); | ||||||||||||||
caml_try_run_on_all_domains(stw_terminate_domain, &myself, NULL); | ||||||||||||||
} | ||||||||||||||
Comment on lines
+1650
to
+1653
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Does this work? It avoids hijacking |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/* To avoid any risk of forgetting an action through a race, | ||||||||||||||
[caml_reset_young_limit] is the only way (apart from setting | ||||||||||||||
young_limit to -1 for immediate interruption) through which | ||||||||||||||
|
@@ -1972,8 +2001,7 @@ static void domain_terminate (void) | |||||||||||||
/* signal the domain termination to the backup thread | ||||||||||||||
NB: for a program with no additional domains, the backup thread | ||||||||||||||
will not have been started */ | ||||||||||||||
atomic_store_release(&domain_self->backup_thread_msg, BT_TERMINATE); | ||||||||||||||
caml_plat_signal(&domain_self->domain_cond); | ||||||||||||||
terminate_backup_thread(domain_self); | ||||||||||||||
caml_plat_unlock(&domain_self->domain_lock); | ||||||||||||||
|
||||||||||||||
caml_plat_assert_all_locks_unlocked(); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,7 @@ CAML_RUNTIME_EVENTS_DESTROY(); | |
#ifndef NATIVE_CODE | ||
caml_debugger(PROGRAM_EXIT, Val_unit); | ||
#endif | ||
caml_terminate_all_domains(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that But then it is not clear to me whether immediately terminating all domains is always the expected behaviour of |
||
if (caml_params->cleanup_on_exit) | ||
caml_shutdown(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be preferable to do this regardless of whether cleanup will occur, in order not to leave "dangling" threads in programs embedding the caml runtime (as mentioned in the PR description). But if people are worried of the consequences of this change, I'll move it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't personally have a strong opinion -- I don't know about threads and systems programming enough to tell the amount of risk involved here -- so I am happy to follow your preference unless someone else speaks out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the one hand, this adds some time overhead. On the other hand, I think there is a race in the existing code between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure this call to I am not sure I understand the point about programs embedding the ocaml runtime. These programs will indeed call |
||
#ifdef _WIN32 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see other comment)