Skip to content

Prevent a race condition in testHook#7831

Merged
fuweid merged 1 commit intocontainerd:mainfrom
kzys:fix-race
Dec 20, 2022
Merged

Prevent a race condition in testHook#7831
fuweid merged 1 commit intocontainerd:mainfrom
kzys:fix-race

Conversation

@kzys
Copy link
Copy Markdown
Member

@kzys kzys commented Dec 16, 2022

The logger could be called from multiple goroutines, but t.Log() is not designed for.

Signed-off-by: Kazuyoshi Kato [email protected]

Prevent a race condition in testHook
7914280 
The logger could be called from multiple goroutines,
but t.Log() is not designed for.

Signed-off-by: Kazuyoshi Kato <[email protected]>
@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Dec 16, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kzys
Copy link
Copy Markdown
Member Author

kzys commented Dec 16, 2022 •
edited
Loading

https://github.com/containerd/containerd/actions/runs/3711243755/jobs/6291512984 was having this race condition

=== Failed
=== FAIL: . TestTransferEcho/ImportExportEchoEmpty (0.00s)
    transfer_test.go:50: rpc error: code = NotFound desc = not found
    --- FAIL: TestTransferEcho/ImportExportEchoEmpty (0.00s)
==================
WARNING: DATA RACE
Read at 0x00c000006ee3 by goroutine 3553:
  testing.(*common).logDepth()

=== FAIL: . TestTransferEcho (0.02s)
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:883 +0xc4
  testing.(*common).log()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:876 +0x84
  testing.(*common).Log()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:917 +0x5b
  testing.(*T).Log()
      <autogenerated>:1 +0x55
  github.com/containerd/containerd/log/logtest.(*testHook).Fire()
      /home/runner/work/containerd/containerd/log/logtest/log_hook.go:40 +0x119
  github.com/sirupsen/logrus.LevelHooks.Fire()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/hooks.go:28 +0xbb
  github.com/sirupsen/logrus.(*Entry).fireHooks()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:280 +0x316
  github.com/sirupsen/logrus.(*Entry).log()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:242 +0x859
  github.com/sirupsen/logrus.(*Entry).Log()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:304 +0x8b
  github.com/sirupsen/logrus.(*Entry).Logf()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:349 +0xc4
  github.com/sirupsen/logrus.(*Entry).Errorf()
      /home/runner/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:378 +0x387
  github.com/containerd/containerd/pkg/transfer/archive.(*ImageExportStream).MarshalAny.func1()
      /home/runner/work/containerd/containerd/pkg/transfer/archive/exporter.go:65 +0x116

Previous write at 0x00c000006ee3 by goroutine 3527:
  testing.tRunner.func1()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1433 +0x7e4
  runtime.deferreturn()
      /opt/hostedtoolcache/go/1.19.4/x64/src/runtime/panic.go:476 +0x32
  testing.(*T).Run.func1()
      /opt/hostedtoolcache/go/1.19.4/x64/src/testing/testing.go:1493 +0x47

@kzys kzys marked this pull request as ready for review December 16, 2022 23:59
dcantah
dcantah approved these changes Dec 20, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

Nice, just hit this on a PR. LGTM

@fuweid fuweid merged commit dd5605e into containerd:main Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fuweid fuweid fuweid approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

No one assigned

Labels

easy-to-review Easy to review kind/test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kzys @k8s-ci-robot @fuweid @dcantah