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.

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

FailsafeFuture: clear execution after completion #362

Merged
merged 1 commit into from Mar 20, 2023
Merged

FailsafeFuture: clear execution after completion #362

merged 1 commit into from Mar 20, 2023

Conversation

stevenschlansker
Copy link
Contributor

Fixes #361

@jhalterman
Copy link
Member

jhalterman commented Mar 16, 2023 •
edited

Thanks for filing this issue and PR!

Since the reference chain you pointed out is FailsafeFuture -> AsyncExecutionImpl, I'm thinking it would be better to set newestExecution and cancelFunctions to null at the end of the FailsafeFuture.cancel and completeResult methods, since they may hold references to the execution. That doesn't address sync executions, which your PR does, but it doesn't seem like these would be a problem? Are you able to try that and confirm it resolves the problem on your side, and if so, update the PR?

@stevenschlansker
FailsafeFuture: clear execution after completion
63c2eb1 
This should fix result futures strongly retaining their computing functions
@stevenschlansker stevenschlansker changed the title ExecutionImpl: allow GC of supplying function after execution FailsafeFuture: clear execution after completion Mar 20, 2023
@stevenschlansker
Copy link
Contributor Author

Thanks @jhalterman , I made the requested change and I agree this should fix our problem. We don't care about sync executions.

@jhalterman jhalterman merged commit e126efb into failsafe-lib:master Mar 20, 2023
6 checks passed
@jhalterman
Copy link
Member

Thanks @stevenschlansker !

@stevenschlansker stevenschlansker deleted the execution-memory-reclaim branch March 20, 2023 19:54
@jhalterman
Copy link
Member

This was released in Failsafe 3.3.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Result futures strongly retain supplying functions
2 participants
@stevenschlansker @jhalterman