Prevent re-entrant execution of finalizers #10602

JaroslavTulach · 2024-07-19T06:54:16Z

Pull Request Description

Fixes #10211 by avoiding re-entrant execution of finalizers.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

All code follows the
Scala,
Java,
Unit tests have been written where possible.

build/build/src/engine/context.rs

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java

radeusgd

I look at the scheduleFinalizationAtSafepoint method and wonder - does it still do what it says? Looking at the code, the submitThreadLocal call was removed from it. It seems that this method is now actually doing finalizeAndUnregisterFromList or something like that, but not quite scheduleFinalizationAtSafepoint.

Can we get the method name updated to reflect its current meaning? Otherwise it is just misleading

radeusgd · 2024-07-19T08:14:41Z

The Enso tests look good. Out of curiosity, how long does it take to allocate and clean the 100k resources?

JaroslavTulach · 2024-07-19T08:49:31Z

The Enso tests look good. Out of curiosity, how long does it take to allocate and clean the 100k resources?

enso$ time ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/e
nso-0.0.0-dev/bin/enso --run test/Base_Tests/src/Runtime/GC_Example.enso 100000
Allocating 100000 resources...
Cleaning up...
All cleaned up! Remaining: 0
0

real    0m6,252s
user    0m26,125s
sys     0m3,088s

vs.

enso$ time ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run test/Base_Tests/src/Runtime/GC_Example.enso 1
Allocating 1 resources...
Cleaning up...
All cleaned up! Remaining: 0
0

real    0m5,300s
user    0m18,851s
sys     0m2,424s

radeusgd · 2024-07-19T11:16:18Z

Looks good but I think the scheduleFinalizationAtSafepoint method should be renamed to reflect what it is actually doing. Unless I highly misunderstood something?

JaroslavTulach · 2024-07-19T13:38:25Z

Can we get the method name updated to reflect its current meaning? Otherwise it is just misleading

Let's remove the method altogether. Then we don't need to care about naming: 0cc2288!

radeusgd

Thanks for addressing the misleading method name, it looks better now.

Akirathan

I don't understand the code entirely, but I trust the tests.

JaroslavTulach · 2024-07-22T07:50:29Z

There is a test failure:

should report only a limited number of warnings for incomparable values on all platforms

Reason: (sorted - warnings = [Different comparators: [
  Standard.Base.Internal.Ordering_Helpers.Default_Comparator], Values NaN and 162 are incomparable, 
  Values 00:00:00 and Date.type.new[Date.enso:103-105] self=Date year=_ are incomparable, 
  Values 429 and NaN are incomparable, Values 319 and 'foo261' are incomparable, 
  Values 'foo261' and 259 are incomparable, Values 242 and 'foo241' are incomparable, 
  Values 00:00:00 and Nothing are incomparable, Values 'foo451' and Nothing are incomparable, 
  Values [] and 392 are incomparable, Values 112 and NaN are incomparable
]) 11 did not equal 10 (at /Users/runner/work/enso/enso/test/Base_Tests/src/Data/Vector_Spec.enso:917:13-45).

radeusgd · 2024-07-22T08:14:18Z

There is a test failure:

should report only a limited number of warnings for incomparable values on all platforms

Reason: (sorted - warnings = [Different comparators: [
  Standard.Base.Internal.Ordering_Helpers.Default_Comparator], Values NaN and 162 are incomparable, 
  Values 00:00:00 and Date.type.new[Date.enso:103-105] self=Date year=_ are incomparable, 
  Values 429 and NaN are incomparable, Values 319 and 'foo261' are incomparable, 
  Values 'foo261' and 259 are incomparable, Values 242 and 'foo241' are incomparable, 
  Values 00:00:00 and Nothing are incomparable, Values 'foo451' and Nothing are incomparable, 
  Values [] and 392 are incomparable, Values 112 and NaN are incomparable
]) 11 did not equal 10 (at /Users/runner/work/enso/enso/test/Base_Tests/src/Data/Vector_Spec.enso:917:13-45).

Related ticket: #10610

hubertp · 2024-07-22T13:47:13Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java

+ for (; ; ) {
+ Item[] toProcess;
+ synchronized (pendingItems) {
+ request.cancel(false);


request is guaranteed to be non-null at this point?

It should be non-null. To get into this process method, a call to submitThreadLocal must be made and it assigns the request.

The request is only assigned back to null in this method, just before return - after this check.

E.g. unless there is some re-entrant invocation of the process method (it was there, but I hopefully fixed it), request shall not be null at this point.

Yes, I can see that gets set there along with adding to pendingItems. But it wasn't obvious that perform can't be called with an empty pendingItems and then it would crash.

Shall we maybe include an assert at least?

What's a difference between NullPointerException and AssertionError?

Usually the NPE can be delayed and happen somewhere down the line, making debugging harder.

In this case I guess you are right - no meaningful difference. I just don't like NPEs so that was by habit :)

… comparators than one

JaroslavTulach · 2024-07-25T07:02:50Z

test/Base_Tests/src/Runtime/Managed_Resource_Spec.enso

@@ -57,6 +58,10 @@ add_specs suite_builder = suite_builder.group "Managed_Resource" group_builder->
 r_3 = Panic.recover Any <| Managed_Resource.bracket 42 (_-> Nothing) (_-> Panic.throw "action")
 r_3.catch . should_equal "action"

+ group_builder.specify "allocate lots of resources at once" <|


This test was disabled by

Disable problematic example #10642

Attempt to diagnose what the problem was is at

Avoid race condition in GC_Example test #10665

JaroslavTulach · 2024-07-25T07:09:24Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java

+ it.flaggedForFinalization.set(true);
+ synchronized (pendingItems) {
+ if (request == null) {
+ request = context.submitThreadLocal(null, this);


Here is the submitThreadLocal javadoc.

If the threads array is null then the thread local action will be performed on all alive threads

Right now, when we run single threaded, one thread will pick the action. In the future, multiple threads may execute the perform method. In such situation the request may actually become null for some (slower) threads.

Using recurring events should be preferred

The ProcessItems constructor marks the ThreadLocalAction as recurring to make sure some thread will pick our action up.

ThreadLocalAction javadoc is also available.

Asynchronous thread-local actions might start and complete to perform independently of each other.

Yes, we want asynchronous action, as we only want to run the action on a single thread. We don't care about others.

Continues at the next PR.

JaroslavTulach added 2 commits July 19, 2024 07:10

Fixing imports in RuntimeManagementTest

20001fb

Prevent re-entrant invocation of finalizers

d40caa1

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 19, 2024

JaroslavTulach self-assigned this Jul 19, 2024

JaroslavTulach requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, marthasharkey, 4e6, hubertp and Akirathan as code owners July 19, 2024 06:54

Remove built-distribution debris before invoking build

9977a56

JaroslavTulach requested a review from Frizi as a code owner July 19, 2024 07:38

JaroslavTulach commented Jul 19, 2024

View reviewed changes

build/build/src/engine/context.rs Show resolved Hide resolved

radeusgd reviewed Jul 19, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java Show resolved Hide resolved

radeusgd reviewed Jul 19, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/ResourceManager.java Outdated Show resolved Hide resolved

radeusgd reviewed Jul 19, 2024

View reviewed changes

Removing scheduleFinalizationAtSafepoint method

0cc2288

JaroslavTulach requested a review from radeusgd July 19, 2024 13:38

radeusgd approved these changes Jul 19, 2024

View reviewed changes

GregoryTravis approved these changes Jul 19, 2024

View reviewed changes

Akirathan approved these changes Jul 19, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jul 20, 2024

StackOverflow when multiple Managed Resources are being cleaned up at the same time #10211

Closed

JaroslavTulach added 2 commits July 21, 2024 08:59

Cancel safe point action as soon as it running

3c23421

Use request as an indicator of finished request

8ad8117

Merge remote-tracking branch 'origin/develop' into wip/jtulach/Gc10211

105bf72

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jul 22, 2024

hubertp approved these changes Jul 22, 2024

View reviewed changes

Only attach different comparator warning when there is more groups of…

7082e49

… comparators than one

JaroslavTulach linked an issue Jul 22, 2024 that may be closed by this pull request

Unexpected comparator warning in Vector_Spec #10610

Closed

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jul 22, 2024

mergify bot merged commit b6bbfc5 into develop Jul 22, 2024
42 checks passed

mergify bot deleted the wip/jtulach/Gc10211 branch July 22, 2024 20:11

JaroslavTulach commented Jul 25, 2024

View reviewed changes

JaroslavTulach mentioned this pull request Jul 26, 2024

Avoid race condition in GC_Example test #10665

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent re-entrant execution of finalizers #10602

Prevent re-entrant execution of finalizers #10602

JaroslavTulach commented Jul 19, 2024 •

edited

Loading

radeusgd left a comment

radeusgd commented Jul 19, 2024

JaroslavTulach commented Jul 19, 2024 •

edited

Loading

radeusgd commented Jul 19, 2024

JaroslavTulach commented Jul 19, 2024

radeusgd left a comment

Akirathan left a comment

JaroslavTulach commented Jul 22, 2024 •

edited

Loading

radeusgd commented Jul 22, 2024

hubertp Jul 22, 2024

JaroslavTulach Jul 22, 2024 •

edited

Loading

hubertp Jul 22, 2024 •

edited

Loading

radeusgd Jul 22, 2024

JaroslavTulach Jul 25, 2024

radeusgd Jul 25, 2024

JaroslavTulach Jul 25, 2024 •

edited

Loading

JaroslavTulach Jul 25, 2024 •

edited

Loading

JaroslavTulach Jul 25, 2024 •

edited

Loading

Prevent re-entrant execution of finalizers #10602

Prevent re-entrant execution of finalizers #10602

Conversation

JaroslavTulach commented Jul 19, 2024 • edited Loading

Pull Request Description

Checklist

radeusgd left a comment

Choose a reason for hiding this comment

radeusgd commented Jul 19, 2024

JaroslavTulach commented Jul 19, 2024 • edited Loading

radeusgd commented Jul 19, 2024

JaroslavTulach commented Jul 19, 2024

radeusgd left a comment

Choose a reason for hiding this comment

Akirathan left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Jul 22, 2024 • edited Loading

radeusgd commented Jul 22, 2024

hubertp Jul 22, 2024

Choose a reason for hiding this comment

JaroslavTulach Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

hubertp Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

radeusgd Jul 22, 2024

Choose a reason for hiding this comment

JaroslavTulach Jul 25, 2024

Choose a reason for hiding this comment

radeusgd Jul 25, 2024

Choose a reason for hiding this comment

JaroslavTulach Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach commented Jul 19, 2024 •

edited

Loading

JaroslavTulach commented Jul 19, 2024 •

edited

Loading

JaroslavTulach commented Jul 22, 2024 •

edited

Loading

JaroslavTulach Jul 22, 2024 •

edited

Loading

hubertp Jul 22, 2024 •

edited

Loading

JaroslavTulach Jul 25, 2024 •

edited

Loading

JaroslavTulach Jul 25, 2024 •

edited

Loading

JaroslavTulach Jul 25, 2024 •

edited

Loading