hashicorp/raft

Push to the notify channel and shutdown sign arriving,it is pick one of two but actually nobody know which one

SchopenhauerZhang opened this issue · 5 comments

i test code found it, i am not sure it is a bug or it is designned; but it is weird;

When Push to the notify channel ,received shutdown sign.we know it is pick one of two,but actually run which one,God knows.

follow the code ,https://github.com/hashicorp/raft/blob/main/raft.go#L434

notify := r.config().NotifyCh

	// Push to the notify channel if given
	if notify != nil {
		select {
		case notify <- true:
		case <-r.shutdownCh:
		}
	}

Hi @SchopenhauerZhang,

You're right that this behaviour is non-deterministic, but that's ok here. When we're shutting down, possibly we won't send the message, but we're ok with that. Why do you think it's a problem that we may not send the notification during shutdown?

@ncabatoff
Sorry, I am very slow to reply, there are too many daily chores.
I recall that when I first discovered this problem, my thoughts were:
During local testing, I found that the behavior here was unknowable, which caused trouble for the test. As a user, I still hope that every behavior is clear and try to avoid it (Ambiguous behavior). Ambiguous behavior should be avoided in programming and coding, right? So I think there's something wrong here.
Of course, I still hope that everyone will discuss and criticize more, and my thinking may not be perfect.

I still need to see ,Will it cause other unknown problems, such as:
if this problem will cause a piece of logic to be lost in the shutdown phase.
Also,:-)
during testing ,i found can not send the notification during shutdown. But I hope Complete the process (such as send the notification ) during shutdown. If you read the source code carefully, you will find that it is also designed in this way.:-)

My English is poor,Please remind me if I didn't explain clearly.
(By the way ,I used Google Translation :-| )

banks commented

Thanks for the PR @SchopenhauerZhang!

It looks like that's merged so I'm going to close this issue now. Thanks for your contribution ❤️ .