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 the memory leak in event handler #967

Merged
merged 1 commit into from
Sep 7, 2017
Merged

Conversation

richardpen
Copy link

@richardpen richardpen commented Sep 6, 2017

Summary

fix the memory leak in event handler, #798

Implementation details

Remove the entry in the map after calling sumbitstatechange.

Testing

  • Builds on Linux (make release)
  • Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
  • Unit tests on Linux (make test) pass
  • Unit tests on Windows (go test -timeout=25s ./agent/...) pass
  • Integration tests on Linux (make run-integ-tests) pass
  • Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
  • Functional tests on Linux (make run-functional-tests) pass
  • Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:
yes

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License:
yes

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

## UNRELEASED
* Feature - Support for provisioning Tasks with ENIs
* Bug - Fixed an memory leak issue
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add more details here? "Fixed a memory leak issue when ..."

// Prepares a given event to be sent by adding it to the handler's appropriate
// eventList
// addEvent prepares a given event to be sent by adding it to the handler's appropriate
// eventList and remove the entry in tasksToEvents map
func (handler *TaskHandler) addEvent(change *sendableEvent, client api.ECSClient) {
seelog.Info("TaskHandler, Adding event: ", change)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this below the taskHandlerLock statements? Also, you can change it to use Infof while you're here.

Copy link
Contributor

@adnxn adnxn left a comment

Choose a reason for hiding this comment

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

just @aaithal's comment about moving the log line below the lock. otherwise lgtm.

@richardpen richardpen merged commit bc33c22 into aws:dev Sep 7, 2017
@jhaynes jhaynes mentioned this pull request Sep 30, 2017
@richardpen richardpen deleted the issue-798 branch November 21, 2017 01:02
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.

4 participants