Skip to content

Set Schedulers removeOnCancelPolicy to avoid task memory leak #1674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 3, 2019
Merged

Set Schedulers removeOnCancelPolicy to avoid task memory leak #1674

merged 3 commits into from
May 3, 2019
+2 −0

Conversation

aftersss
Copy link

@aftersss aftersss commented Apr 24, 2019 •
edited
Loading

If we run the following code(with jvm args -Xmx10m -Xms10m), we will soon meet java.lang.OutOfMemoryError: GC overhead limit exceeded:

                Scheduler scheduler = Schedulers.newParallel("a",100);

		for(int i=0;i<1000000;i++){
			Mono<String> mono = Mono.delay(Duration.ofMillis(10), scheduler)
					.map(aLong -> "a")
					.timeout(Duration.ofMillis(60000), Mono.just("b"));


			mono.subscribe(s -> {
			});
		}

And I found that the key point is that ScheduledThreadPoolExecutor.removeOnCancelPolicy's default value is false, I think the default value should be true to avoid memory leak.

@codecov-io
Copy link

codecov-io commented Apr 24, 2019 •
edited
Loading

Codecov Report

Merging #1674 into 3.2.x will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##              3.2.x    #1674      +/-   ##
============================================
+ Coverage     84.29%   84.31%   +0.02%     
- Complexity     3908     3913       +5     
============================================
  Files           360      360              
  Lines         29940    29957      +17     
  Branches       5573     5575       +2     
============================================
+ Hits          25237    25259      +22     
  Misses         3072     3072              
+ Partials       1631     1626       -5
Impacted Files Coverage Δ Complexity Δ
...java/reactor/core/scheduler/ParallelScheduler.java 78.57% <100%> (+0.31%) 24 <0> (ø) ⬇️
.../java/reactor/core/scheduler/ElasticScheduler.java 83.6% <100%> (+0.13%) 26 <0> (ø) ⬇️
...ava/reactor/core/publisher/WorkQueueProcessor.java 71.48% <0%> (-4.14%) 19% <0%> (ø)
...ain/java/reactor/core/publisher/MonoPublishOn.java 85.71% <0%> (-3.18%) 3% <0%> (ø)
...main/java/reactor/core/publisher/FluxRefCount.java 85.71% <0%> (-2.2%) 15% <0%> (-1%)
...ain/java/reactor/util/concurrent/WaitStrategy.java 50.42% <0%> (-1.71%) 11% <0%> (ø)
.../java/reactor/core/publisher/BlockingIterable.java 77.08% <0%> (-1.05%) 7% <0%> (ø)
...ain/java/reactor/core/publisher/FluxPublishOn.java 86.39% <0%> (-0.21%) 6% <0%> (ø)
...java/reactor/core/publisher/FluxBufferTimeout.java 77.85% <0%> (ø) 3% <0%> (ø) ⬇️
...rc/main/java/reactor/core/publisher/Operators.java 80.89% <0%> (+0.08%) 123% <0%> (+2%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab95105...518fb9d. Read the comment docs.

smaldini
smaldini approved these changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@smaldini smaldini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we might want to benchmark with that - WDYT @akarnokd ? I guess it has some impact and that's why we might explore a separate batch-purge thread as well.

@akarnokd
Copy link
Contributor

Purge is more efficient but you need a periodic task possibly on another scheduled executor to initiate cleanup.

@aftersss
Copy link
Author

aftersss commented Apr 30, 2019 •
edited
Loading

I have committed some new changes that purge ScheduledThreadPoolExecutor on another scheduled executor. Maybe this is better?

simonbasle
simonbasle suggested changes May 3, 2019
View reviewed changes
Copy link
Contributor

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to revert the last commit and go back to the cancellation policy implementation for a start

@simonbasle
Copy link
Contributor

@aftersss we can go back to the purge-thread based version at a later time, maybe with some microbenchmarking to see how much of a difference it makes to see if the added complexity is worth it.

@simonbasle simonbasle added this to the 3.3.0.RELEASE milestone May 3, 2019
@simonbasle simonbasle modified the milestones: 3.3.0.RELEASE, 3.3.0.M1 May 3, 2019
@simonbasle simonbasle changed the title Set ScheduledThreadPoolExecutor.removeOnCancelPolicy to true to avoid memory leak. Set Schedulers removeOnCancelPolicy to avoid task memory leak May 3, 2019
@simonbasle simonbasle merged commit 08bfaea into reactor:3.2.x May 3, 2019
@reactorbot
Copy link

@aftersss this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to master 🙇

@simonbasle simonbasle modified the milestones: 3.3.0.M1, 3.2.9.RELEASE May 3, 2019
simonbasle added a commit that referenced this pull request May 3, 2019
@simonbasle
Merge #1674 into 3.3
de03f6c
smaldini pushed a commit that referenced this pull request May 6, 2019
Set Schedulers removeOnCancelPolicy to avoid task memory leak (#1674)
76be9bb 
Although a purge background thread could be more efficient, it would
also be more complex, so this is a good first approach.
@smaldini smaldini mentioned this pull request May 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smaldini smaldini

@simonbasle simonbasle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.9.RELEASE
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aftersss @codecov-io @akarnokd @simonbasle @reactorbot @smaldini