Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Fix the bug in unit tests about sync.Pool #1426

Merged
merged 1 commit into from
Jul 14, 2020

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented Jul 13, 2020

Ⅰ. Describe what this PR did

Callers should not assume any relation between values passed to Put and the values returned by Get.

Ref: https://github.com/golang/go/blob/master/src/sync/pool.go#L116-L151

// Get selects an arbitrary item from the Pool, removes it from the
// Pool, and returns it to the caller.
// Get may choose to ignore the pool and treat it as empty.
// Callers should not assume any relation between values passed to Put and
// the values returned by Get.
//
// If Get would otherwise return nil and p.New is non-nil, Get returns
// the result of calling p.New.
func (p *Pool) Get() interface{} {

Ref: https://github.com/golang/go/blob/master/src/sync/pool_test.go#L28-L42

	// Make sure that the goroutine doesn't migrate to another P
	// between Put and Get calls.
	Runtime_procPin()
	p.Put("a")
	p.Put("b")
	if g := p.Get(); g != "a" {
		t.Fatalf("got %#v; want a", g)
	}
	if g := p.Get(); g != "b" {
		t.Fatalf("got %#v; want b", g)
	}
	if g := p.Get(); g != nil {
		t.Fatalf("got %#v; want nil", g)
	}
	Runtime_procUnpin()

It seems that we should pin the P when do the unit test :)

Ⅱ. Does this pull request fix one issue?

Fixes: #1416

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

make unit-test

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added areas/test kind/bug This is bug report for project size/S labels Jul 13, 2020
@SataQiu SataQiu changed the title Fix the bug in unit tests about sync.Pool WIP: Fix the bug in unit tests about sync.Pool Jul 13, 2020
@YesterdayxD
Copy link
Contributor

#1418 @SataQiu

@lowzj
Copy link
Member

lowzj commented Jul 13, 2020

It's caused by the race detecting of go test -race. It has 1/4 possibility to drop the value in sync.Pool.Put function.

@SataQiu SataQiu force-pushed the fix-pool-test-20200713 branch from 950092e to 8d9596e Compare July 14, 2020 06:36
@@ -1,3 +1,6 @@
// BufferPool is no-op under race detector, so all these tests do not work.
// +build !race

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It's a workaround solution, but it will not run unit-testing of this file in current CI flow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can do something in hack/unit-test.sh, such as adding -race dynamically for go test :)

for d in $(eval "${cmd}")
do
go test -race -coverprofile=profile.out -covermode=atomic "${d}"
if [ "$?" != "0" ]; then

Comment on lines +58 to +62
// Limit to 1 processor to make sure that the goroutine doesn't migrate
// to another P between AcquireBuffer and ReleaseBuffer calls.
prev := runtime.GOMAXPROCS(1)
defer runtime.GOMAXPROCS(prev)

Copy link
Member Author

Choose a reason for hiding this comment

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

@SataQiu SataQiu changed the title WIP: Fix the bug in unit tests about sync.Pool Fix the bug in unit tests about sync.Pool Jul 14, 2020
Copy link
Member

@lowzj lowzj left a comment

Choose a reason for hiding this comment

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

LGTM


I will merge this to make the CI pass.
If there is a better solution, please submit another pull request.

@lowzj lowzj merged commit 12cfd62 into dragonflyoss:master Jul 14, 2020
lowzj added a commit to lowzj/Dragonfly that referenced this pull request Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
areas/test kind/bug This is bug report for project size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unit test failed in all probability in CI environment when pull request
4 participants