Skip to content

Fix memory leak#13892

Merged
sm6srw merged 1 commit intoDynamoDS:masterfrom
sm6srw:fix_mem_leak
Apr 11, 2023
Merged

Fix memory leak#13892
sm6srw merged 1 commit intoDynamoDS:masterfrom
sm6srw:fix_mem_leak

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