internal/grpcsync: support two ways to schedule a callback with the serializer

[easwars]

The earlier API to add callbacks to the serializer, Schedule, was returning a bool to indicate whether or not the callback was added to the serializer. This had the following problems:

• No way to mandate that callers actually look at the return value
• Downstream errors because of the failure to schedule a callback (and the failure to look at the return value) can be very hard to debug
• Very few callers need to take specific action when the call to Schedule fails, while for most of the others, it is totally OK for the call to Schedule to fail.

This PR splits the API into two:

• MaybeSchedule: A best effort API that fails silently if the serializer is closed
• ScheduleOr: An API which runs an onFailure func if the callback cannot be scheduled

RELEASE NOTES: none

[codecov]

Codecov Report

Attention: Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.38%. Comparing base (e7d8822) to head (343432d).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7408      +/-   ##
==========================================
- Coverage   81.45%   81.38%   -0.08%     
==========================================
  Files         348      350       +2     
  Lines       26752    26845      +93     
==========================================
+ Hits        21792    21848      +56     
- Misses       3773     3799      +26     
- Partials     1187     1198      +11     

Files	Coverage Δ
balancer_wrapper.go	88.74% <100.00%> (+1.98%)	⬆️
internal/grpcsync/callback_serializer.go	100.00% <100.00%> (ø)
internal/grpcsync/pubsub.go	76.47% <100.00%> (ø)
resolver_wrapper.go	85.10% <100.00%> (ø)
...s/internal/balancer/cdsbalancer/cluster_watcher.go	100.00% <100.00%> (ø)
...rnal/balancer/clusterresolver/resource_resolver.go	93.33% <100.00%> (ø)
xds/internal/resolver/serviceconfig.go	88.14% <100.00%> (ø)
xds/internal/xdsclient/authority.go	86.09% <100.00%> (ø)
xds/internal/resolver/watch_service.go	89.47% <83.33%> (ø)
xds/internal/xdsclient/clientimpl_watchers.go	76.74% <50.00%> (ø)
... and 1 more

... and 26 files with indirect coverage changes

[dfawley]

This LGTM but one minor simplification suggestion. Naming question: Maybe is OK and what I originally proposed, but how do you feel about TrySchedule?

[dfawley]

This can stay unbounded, and maybe improved a little:

func uCCS() {
	errCh := make(chan error)
	defer close(errCh)
	uccs := ...
	onFailure := func() {}
...

[easwars]

We need to block on reading from this channel when the uccs callback is successfully scheduled on the serializer (because we want to handle state updates inline). But if we don't send something on the channel from the onFailure func, the last line return <-errCh will block forever, right? This is equivalent, isn't it? https://go.dev/play/p/GHPD4MLhis6

[dfawley]

Um, yes, I tried to simplify too much, oops. You can still make the channel unbuffered (sorry for saying "unbounded"), and not defer the close but call close in the onFailure:

https://go.dev/play/p/oqDNkOs_hj7

[easwars]

Done.

[dfawley]

Similar here as above.

[easwars]

I can't make it unbuffered here because if ScheduleOr fails, it will call onFailure inline and this means that the write to the channel on line 327 errCh <- errBalancerClosed will happen before the read on line 330. Therefore it will just hang.

[dfawley]

Ah yes, that makes sense.