Skip to content

Fix memory leak #13892

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
sm6srw merged 1 commit into from
Apr 11, 2023
Merged

Fix memory leak #13892

sm6srw merged 1 commit into from
Apr 11, 2023

Conversation

@sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Apr 11, 2023 •
edited
Loading

Purpose

The logger constructors where called twice:

  • This fixes the memory issue that caused the serial tests to timeout.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Reviewers

FYIs

@sm6srw sm6srw added the WIP label Apr 11, 2023
mjkkirschner
mjkkirschner reviewed Apr 11, 2023
View reviewed changes
src/DynamoCore/Logging/DynamoLogger.cs
/// <param name="isServiceMode">We want restrict logging in service mode to console only due to lambda limitations.</param>
/// TODO(DYN-5757): Review usage of isTestMode,isTestMode,isServiceMode across Dynamo and see how we can consildate all these flags.
public DynamoLogger(DebugSettings debugSettings, string logDirectory, Boolean isTestMode, Boolean isCLIMode, Boolean isServiceMode)
: this(debugSettings, logDirectory, isTestMode)
Copy link
Member

@mjkkirschner mjkkirschner Apr 11, 2023

Choose a reason for hiding this comment

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

ahh, did this sub LogToConsole twice?

Copy link
Contributor Author

@sm6srw sm6srw Apr 11, 2023

Choose a reason for hiding this comment

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

Yes

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 11, 2023

Parallel tests passed here: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/8812/

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 11, 2023

Serial tests passed here: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/1141/

@sm6srw sm6srw removed the WIP label Apr 11, 2023
@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 11, 2023

@mjkkirschner PTAL

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 11, 2023 •
edited
Loading

@sm6srw is that event unsubscribed anywhere? (originally I mean, even before the PR that introduced the issue)

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 11, 2023

Yes, it is. That was the leak. We subscribed twice but was only unsubscribing once.

@sm6srw
Copy link
Contributor Author

sm6srw commented Apr 11, 2023

@mjkkirschner This new constructor was introduced here: #13860

@sm6srw sm6srw merged commit 952e380 into DynamoDS:master Apr 11, 2023
@sm6srw sm6srw deleted the fix_mem_leak branch April 11, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjkkirschner mjkkirschner mjkkirschner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sm6srw @mjkkirschner