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

Do not check pending interrupts when running finalizers #4366

Merged
merged 1 commit into from Jul 29, 2021

Conversation

jeremyevans
Copy link
Contributor

This fixes cases where exceptions raised using Thread#raise are
swallowed by finalizers and not delivered to the running thread.

This could cause issues with more finalizers that rely on
pending interrupts.

Fixes [Bug #13876]

@jeremyevans
Copy link
Contributor Author

All of Compliations CI failures look Docker-related:

error pulling image configuration: denied: unauthenticated: User cannot be authenticated with the token provided.

This passed CI in my branch: https://github.com/jeremyevans/ruby/runs/2290283774

@ko1
Copy link
Contributor

ko1 commented Jul 29, 2021

How about to use interrupt mask?

The following patch is for this PR and tests on my machine passed.

diff --git a/gc.c b/gc.c
index ef1c23e325..f6ea4fc6de 100644
--- a/gc.c
+++ b/gc.c
@@ -4088,13 +4088,14 @@ static void
 finalize_deferred(rb_objspace_t *objspace)
 {
     VALUE zombie;
-    rb_thread_t *current_th = GET_THREAD();
-    current_th->running_finalizer = 1;
+    rb_execution_context_t *ec = GET_EC();
+    ec->interrupt_mask |= PENDING_INTERRUPT_MASK;
 
     while ((zombie = ATOMIC_VALUE_EXCHANGE(heap_pages_deferred_final, 0)) != 0) {
 	finalize_list(objspace, zombie);
     }
-    current_th->running_finalizer = 0;
+
+    ec->interrupt_mask &= ~PENDING_INTERRUPT_MASK;
 }
 
 static void
diff --git a/thread.c b/thread.c
index 7533935b37..3c3f645dbd 100644
--- a/thread.c
+++ b/thread.c
@@ -2094,7 +2094,7 @@ threadptr_pending_interrupt_active_p(rb_thread_t *th)
      * if the queue and the thread interrupt mask were not changed
      * since last check.
      */
-    if (th->pending_interrupt_queue_checked || th->running_finalizer) {
+    if (th->pending_interrupt_queue_checked) {
 	return 0;
     }
 
diff --git a/vm_core.h b/vm_core.h
index 2273ae5ec2..8ec03d7ec2 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -973,7 +973,6 @@ typedef struct rb_thread_struct {
     unsigned int abort_on_exception: 1;
     unsigned int report_on_exception: 1;
     unsigned int pending_interrupt_queue_checked: 1;
-    unsigned int running_finalizer: 1;
     int8_t priority; /* -3 .. 3 (RUBY_THREAD_PRIORITY_{MIN,MAX}) */
     uint32_t running_time_us; /* 12500..800000 */
 

@jeremyevans
Copy link
Contributor Author

@ko1 That approach using interrupt_mask looks less hacky than my approach. I'll switch to that and rebase against master. Assuming CI passes, I'll merge this.

This fixes cases where exceptions raised using Thread#raise are
swallowed by finalizers and not delivered to the running thread.

This could cause issues with finalizers that rely on pending interrupts,
but that case is expected to be rarer.

Fixes [Bug #13876]
Fixes [Bug #15507]

Co-authored-by: Koichi Sasada <ko1@atdot.net>
@jeremyevans jeremyevans merged commit 87b327e into ruby:master Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants