fix(example): potentional race-condition in consumer-group example

[mabrarov]

Fix race-condition in consumer-group example when waiting for consumer readiness. Refer to #1404 (comment).

[puellanivis]

As noted, no, there was not a potential race condition in the previous example. No, not even a potential for one.

If we’re going to fix this code, then we should do a full rewrite with signal.NotifyContext setup from the start, and we don’t need a whole complicated sync.OnceXY pattern because there’s only one place that might ever close the channel, and it’s only ever possible that a single thread could ever be in that code already… but like I explained, we don’t need that ready condition anyways.

We can just go straight away to the wait for NotifyContext to close. There’s no reason to have to wait for the consumer to be ready. Entirely removing all of this code means everything still works.

[mabrarov]

Hi @puellanivis,

I am under impression that waiting for ready condition in this example is used to demonstrate what sarama.ConsumerGroupHandler.Setup can be used for.

Regarding lack of race-condition - I replied in #1404 (comment) - it's more about correctness than about reproducible bug (which is usual for the issues of race-condition nature).

Thank you.

[puellanivis]

I am under impression that waiting for ready condition in this example is used to demonstrate what sarama.ConsumerGroupHandler.Setup can be used for.

This is still not a proper reason for why it waits to setup the signal handler. It’s absurd that we will simply crash out on a SIGTERM during a partial startup rather than unwinding and gracefully shutdown, which is the whole point of having a signal handler handling SIGTERM in the first place.

[mabrarov]

Considering #1404 (comment) - if we still want to keep waiting for sarama.ConsumerGroupHandler.Setup to complete (refer to #3674 (comment))... which is not the thing this PR makes decision on (is out of scope of this PR) - this PR is still helpful to ensure the wait at line 126 (in the new version of the modified main.go) is unblocked even if server-side rebalance happen and sarama.ConsumerGroupHandler.Setup was not called (is it possible?) at the first iteration of loop starting at line 109. Isn't it?

[puellanivis]

sarama.ConsumerGroupHandler.Setup was not called (is it possible?) at the first iteration of loop starting at line 109. Isn't it?

Probably. 🤷‍♀️ We should still clean up everything else. Move the signal handler to start, and then select on the ready, and signal handler, so we can properly handle the case of getting a SIGTERM while setting up as well, and even without moving the signal handler at all, now the context we setup is entirely unnecessary.

There’s nothing wrong with the code as is, but it’s insufficient for the scope of changes that are being made.