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

Add read lock to task string method #4288

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Conversation

mye956
Copy link
Contributor

@mye956 mye956 commented Aug 16, 2024

Summary

This PR will add a read lock to the task object String() method. There seems to be a transient/inconsistent data race condition with this method which was found in our unit tests. For example, one case of this that has happened is in the following run while running TestTaskWithCircularDependency test case.

Error log encountered:

WARNING: DATA RACE
Read at 0x00c000a86e38 by goroutine 11:
  github.com/aws/amazon-ecs-agent/agent/api/task.(*Task).stringUnsafe()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/api/task/task.go:2920 +0x184
  github.com/aws/amazon-ecs-agent/agent/api/task.(*Task).String()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/api/task/task.go:2913 +0x26
  fmt.(*pp).handleMethods()
      /opt/hostedtoolcache/go/1.22.5/x64/src/fmt/print.go:673 +0x6ea
  fmt.(*pp).printArg()
      /opt/hostedtoolcache/go/1.22.5/x64/src/fmt/print.go:756 +0xb4b
  fmt.(*pp).doPrintf()
      /opt/hostedtoolcache/go/1.22.5/x64/src/fmt/print.go:1075 +0x592
  fmt.Sprintf()
      /opt/hostedtoolcache/go/1.22.5/x64/src/fmt/print.go:239 +0x5c
  github.com/cihub/seelog.(*logFormattedMessage).String()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/logger.go:369 +0x75
  github.com/cihub/seelog.(*commonLogger).processLogMsg()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/logger.go:312 +0xca
  github.com/cihub/seelog.(*asyncLogger).processQueueElement()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/behavior_asynclogger.go:115 +0x199
  github.com/cihub/seelog.(*asyncLoopLogger).processItem()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/behavior_asynclooplogger.go:57 +0x1f3
  github.com/cihub/seelog.(*asyncLoopLogger).processQueue()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/behavior_asynclooplogger.go:63 +0x36
  github.com/cihub/seelog.NewAsyncLoopLogger.gowrap1()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/behavior_asynclooplogger.go:40 +0x33

Previous write at 0x00c000a86e38 by goroutine 2015:
  github.com/aws/amazon-ecs-agent/agent/api/task.(*Task).SetDesiredStatus()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/api/task/task.go:2785 +0xa4
  github.com/aws/amazon-ecs-agent/agent/engine.(*DockerTaskEngine).AddTask()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/docker_task_engine.go:1203 +0xf04
  github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph.ValidDependencies()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph/graph.go:101 +0x344
  github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph.dependenciesCanBeResolved()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph/graph.go:130 +0x1b2
  github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph.ValidDependencies()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph/graph.go:101 +0x344
  github.com/aws/amazon-ecs-agent/agent/engine.(*DockerTaskEngine).AddTask()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/docker_task_engine.go:1196 +0xd09
  github.com/aws/amazon-ecs-agent/agent/engine.TestTaskWithCircularDependency.gowrap2()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/docker_task_engine_test.go:1367 +0x50

Goroutine 11 (running) created at:
  github.com/cihub/seelog.NewAsyncLoopLogger()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/behavior_asynclooplogger.go:40 +0x11c
  github.com/cihub/seelog.createLoggerFromFullConfig()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/log.go:73 +0x4b4
  github.com/cihub/seelog.LoggerFromConfigAsBytes()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/cfg_config.go:58 +0xf6
  github.com/cihub/seelog.init.3()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/vendor/github.com/cihub/seelog/log.go:55 +0xe4

Goroutine 2015 (running) created at:
  github.com/aws/amazon-ecs-agent/agent/engine.TestTaskWithCircularDependency()
      /home/runner/work/amazon-ecs-agent/amazon-ecs-agent/src/github.com/aws/amazon-ecs-agent/agent/engine/docker_task_engine_test.go:1367 +0x344
  testing.tRunner()
      /opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.22.5/x64/src/testing/testing.go:1742 +0x44

Implementation details

Testing

Ran the TestTaskWithCircularDependency 500 times and it no longer encountered the race condition issue.

Commit
Linux unit test run

New tests cover the changes: No

Description for the changelog

Bugfix: Add read lock to task object String method

Additional Information

Does this PR include breaking model changes? If so, Have you added transformation functions?

Does this PR include the addition of new environment variables in the README?

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mye956 mye956 requested a review from a team as a code owner August 16, 2024 18:32
@mye956 mye956 force-pushed the read-lock-task-string branch 3 times, most recently from 8b53921 to 7f8ebc7 Compare August 16, 2024 18:49
@mye956 mye956 changed the title [WIP] Add Read lock to task ToString method [WIP] Add Read lock to task String method Aug 16, 2024
@mye956 mye956 changed the title [WIP] Add Read lock to task String method Add Read lock to task String method Aug 16, 2024
@mye956 mye956 changed the title Add Read lock to task String method Add read lock to task string method Aug 16, 2024
@mye956 mye956 force-pushed the read-lock-task-string branch from 7f8ebc7 to 6b44d94 Compare August 16, 2024 21:18
@mye956 mye956 force-pushed the read-lock-task-string branch 3 times, most recently from 25b26c5 to e91cbde Compare August 19, 2024 23:17
@mye956 mye956 force-pushed the read-lock-task-string branch from e91cbde to a9f5236 Compare August 27, 2024 19:13
@mye956 mye956 force-pushed the read-lock-task-string branch from a9f5236 to a71d733 Compare August 28, 2024 15:48
@mye956 mye956 merged commit a876998 into aws:dev Aug 28, 2024
40 checks passed
@Yiyuanzzz Yiyuanzzz mentioned this pull request Sep 20, 2024
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