Do not check pending interrupts when running finalizers#4366
Merged
jeremyevans merged 1 commit intoJul 29, 2021
Conversation
Contributor
Author
|
All of Compliations CI failures look Docker-related: This passed CI in my branch: https://github.com/jeremyevans/ruby/runs/2290283774 |
Contributor
|
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 */
|
Contributor
Author
|
@ko1 That approach using |
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 ruby#13876] Fixes [Bug ruby#15507] Co-authored-by: Koichi Sasada <[email protected]>
jeremyevans
force-pushed
the
finalizer-thread-raise-13876
branch
from
83882c4 to
35a6385
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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]