Skip to content
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

Fix buggy disable_sigint implementation #56977

Open
davidanthoff opened this issue Jan 6, 2025 · 0 comments
Open

Fix buggy disable_sigint implementation #56977

davidanthoff opened this issue Jan 6, 2025 · 0 comments

Comments

@davidanthoff
Copy link
Contributor

From a Slack conversation with @vtjnash it seems that the current implementation of disable_sigint is buggy. I quote him here:

IIUC, defer_signal is inherited from the Task that switched to it, which is a terrible design and not one that we should keep, but is currently what it appears to implement

#49541 (comment) suggests that the intent was (or should be) that the defer_signal state should really be task-local.

Maybe making it properly task-local could count as a simple bug fix that then could also be backported to the LTS version?

I am trying to fix the Ctrl+C handling in the VS Code extension and I think I won't be able to make much progress unless this is fixed.

I believe the buggy code is essentially this

julia/src/task.c

Lines 710 to 740 in 36472a7

// Store old values on the stack and reset
sig_atomic_t defer_signal = ptls->defer_signal;
int finalizers_inhibited = ptls->finalizers_inhibited;
ptls->finalizers_inhibited = 0;
jl_timing_block_t *blk = jl_timing_block_task_exit(ct, ptls);
ctx_switch(ct);
#ifdef MIGRATE_TASKS
ptls = ct->ptls;
t = ptls->previous_task;
ptls->previous_task = NULL;
assert(t != ct);
assert(jl_atomic_load_relaxed(&t->tid) == ptls->tid);
if (!t->sticky && !t->ctx.copy_stack)
jl_atomic_store_release(&t->tid, -1);
#else
assert(ptls == ct->ptls);
#endif
// Pop old values back off the stack
assert(ct == jl_current_task &&
0 != ct->ptls &&
0 == ptls->finalizers_inhibited);
ptls->finalizers_inhibited = finalizers_inhibited;
jl_timing_block_task_enter(ct, ptls, blk); (void)blk;
fesetenv(&ct->fenv);
sig_atomic_t other_defer_signal = ptls->defer_signal;
ptls->defer_signal = defer_signal;
.

I know that there have been a lot of (wide ranging) discussions how SIGINT handling in general could be improved, but in my ideal world those would not hold up a simple bug fix here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant