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

[teraslice-messaging] fix memory leak caused by abortController #3952

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

busma13
Copy link
Contributor

@busma13 busma13 commented Feb 5, 2025

This PR makes the following changes:
-bump teraslice-messaging from 1.10.3 to 1.10.4

  • Remove the abortController from the execution-controller client
  • Create an abortController in the core client handleSendResponse function if sendAbortSignal is set to true. This is only the case for when the execution-controller client sends the worker:slice:complete event. On a server:shutdown event the abortController will abort awaiting a server response so it can shutdown properly.

ref: #3945

@busma13 busma13 requested review from godber and sotojn February 5, 2025 23:31
@busma13 busma13 marked this pull request as ready for review February 5, 2025 23:31
@busma13
Copy link
Contributor Author

busma13 commented Feb 5, 2025

Using the promMetrics exporter I was able to see the resident memory of the worker increasing from ~225MB at. first scrape to ~367MB after 48000 slices on the current master branch. Using this branch the memory increased from ~230MB at first scrape to ~232MB after 72000 slices.

@godber godber merged commit fb41d36 into master Feb 5, 2025
46 checks passed
@godber godber deleted the fix-memory-leak branch February 5, 2025 23:53
@godber
Copy link
Member

godber commented Feb 6, 2025

Unfortunately I didn't catch until now that the teraslice version itself was not bumped. Doesn't bumping teraslice-messaging result in teraslice being bumped?

godber added a commit that referenced this pull request Feb 6, 2025
Bump teraslice version for previous PR
#3952
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