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

chann: unbounded channel may result in a spin loop if there is no receiver after the close #3

Open
Rustin170506 opened this issue Jun 3, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@Rustin170506
Copy link
Contributor

chann/chann.go

Line 176 in 619cda1

default:

If not will it be any problem?

@changkun
Copy link
Member

changkun commented Jun 3, 2022

If there is no receiver, this default branch can prevent the goroutine block forever at the close.

If there is a receiver, the receiver will always receive all unsent data because select will always use case ch.out <- q[0] branch.

@changkun changkun closed this as completed Jun 3, 2022
@changkun changkun added the question Further information is requested label Jun 3, 2022
@Rustin170506
Copy link
Contributor Author

If there is no receiver, this default branch can prevent the goroutine block forever at the close.

Won't the default branch cause a cpu spin if executed?

@changkun changkun reopened this Jun 4, 2022
@changkun
Copy link
Member

changkun commented Jun 4, 2022

Interesting. Looks like a bug. Thanks for reporting!

@changkun changkun added bug Something isn't working and removed question Further information is requested labels Jun 4, 2022
@changkun changkun changed the title Question: Why do we need this default statement? chann: unbounded channel may result in a spin loop if there is no receiver after the close Jun 4, 2022
@changkun
Copy link
Member

changkun commented Jun 8, 2022

I believe ad8c992 did not solve the issue but actually introduced a different bug. Consider this test:

func TestUnboundedChannRecvAfterClose(t *testing.T) {
	var wg sync.WaitGroup
	ch := chann.New[int]()

	wg.Add(1)
	c := 0
	go func() {
		for range ch.Out() {
			c++
		}
		wg.Done()
	}()

	for i := 0; i < 2048; i++ {
		ch.In() <- 42
	}
	ch.Close()

	wg.Wait()
	if c != 2048 {
		t.Fatalf("not all elements are received after channel being closed, want %v got %v", 2048, c)
	}
}

@changkun changkun reopened this Jun 8, 2022
@Rustin170506
Copy link
Contributor Author

How about we just delete deafult? Leaving the decision about the data to the consumer?

@Rustin170506
Copy link
Contributor Author

ch.In() <- 42

Is this a typo?

@changkun
Copy link
Member

changkun commented Jun 9, 2022

How about we just delete deafult? Leaving the decision about the data to the consumer?

Removing the default will lead to leaking goroutine. We must always guarantee the processing goroutine will terminate even if there is no receiver.

@Rustin170506
Copy link
Contributor Author

Then we may have to just close out chan and delete all data immediately.

@changkun
Copy link
Member

changkun commented Jun 9, 2022

Then we may have to just close out chan and delete all data immediately.

Deleting all data immediately may not be a good idea because there may be a receiver waiting for all data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants