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

Fix JoinableTaskContext.serializedTasks memory leak #1346

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Aug 13, 2024

The collection of joinable tasks for which an ID is requested grew forever. The code that should purge completed tasks from the collection was never executing because the TaskId property is programmed to return null for completed tasks, and we query the property directly after setting the task as completed.

The fix then is simple: fetch the TaskId value first and then set the JoinableTask as completed.

@sharwell found this and reports that 361MB was held in memory due to this leak in one dump he investigated.

This fixes a memory leak that impacts versions as far back as 17.6. This PR only applies the fix to 17.12, but the commit itself is based on the original bug so the fix can be merged anywhere necessary.

The collection of joinable tasks for which an ID is requested grew forever. The code that should purge completed tasks from the collection was never executing because the `TaskId` property is programmed to return `null` for completed tasks, and we query the property directly *after* setting the task as completed.

The fix then is simple: fetch the TaskId value *first* and *then* set the `JoinableTask` as completed.
@AArnott AArnott added this to the v17.12 milestone Aug 13, 2024
Copy link
Member

@BertanAygun BertanAygun left a comment

Choose a reason for hiding this comment

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

Nice find!

@AArnott AArnott merged commit 243b960 into microsoft:main Aug 13, 2024
5 checks passed
@AArnott AArnott deleted the fixserializedTaskLeak branch August 13, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants