Cockroach: kvserver: use unambiguous return value in iter callback

[tbg]

See https://github.com/cockroachdb/cockroach/pull/48681 for context.

We have APIs in various places in which we iterate by calling a closure for each item. The closure gets to decide when it is "done" by returning a boolean.

It has always been hard to remember which is which (is true "done" or "more")? In this particular case, it looks like we should just remove the boolean - the iteration stops when an error is returned.

There are a few more iterators with a similar api, for example here:

https://github.com/cockroachdb/cockroach/blob/2e77621302dd5d94706ba0e35a076d481b0f87ae/pkg/kv/kvserver/store.go#L1223

We should take a look at them as well.

[blathers-crl[bot]]

Hi @tbg, please add a C-ategory label to your issue. Check out the label system docs.

While you're here, please consider adding an A- label to help keep our repository tidy.

_{:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[jaibhavaya]

Is this available? If so, I'd like to take a crack at it.

[tbg]

It is!

[jaibhavaya]

Awesome, I’ll take this on! I've joined the cockroachdb slack and will ask any questions there. thanks!

[vrongmeal]

The IterateRangeDescriptors calls fn inside another function that is passed into storage.MVCCIterate (which also takes a similar (done bool, err error) return value function) which does something else for when non-nil error but the iteration is done (unlike in mentioned #48681 where iteration just stops on an error and done has no significance).

If we need to remove the boolean, we need an error to specify that the iteration needs to stop and it is not really an error, something like:

// ErrStopIteration is returned by the closure when the iteration needs to be stopped.
var ErrStopIteration = errors.New("stop iteration")

and then instead of if done check, we can if errors.Is(err, ErrStopIteration).

OR the codebase can be consistent with using done bool instead of more bool. Correct me if I'm wrong but I could not find another instance where more bool is used apart from the example mentioned in your comment:

https://github.com/cockroachdb/cockroach/blob/2e77621302dd5d94706ba0e35a076d481b0f87ae/pkg/kv/kvserver/store.go#L1223

[tbg]

Hey @vrongmeal, thanks for looking into this! My intuition is that we should use the error-based approach you discussed. A boolean will always rely on documentation and is prone to bugs, besides the resulting (bool, error) api is more cluttered.

One problem with the approach is that the sentinel error ErrStopIteration isn't easily discoverable, i.e. if you want to stop early you may have to poke around to learn how to do so. Instead, we could make it such that the closures passed to the iteration get a signature of the kind

func(*iterutil.State) error

where

package iterutil // pkg/util/iterutil?

// State holds the state of an ongoing iteration.
type State struct {
  cur interface{}
  done bool
}

// Cur returns the current element of the iteration.
func (s *State) Cur() interface{} { return s.cur }
// Stop causes the iterator to stop iterating, i.e. the current element is the
// last one to be visited.
func (s *State) Stop() error {
  s.done = true
  return nil
}
// Stopped returns whether Stop was called.
func (s *State) Stopped() bool { return s.done }

and so you would implement the iterator functions (like MVCCIterate, etc) like this instead:

var s iterutil.State
for _, thing := range myThings {
  s.cur = thing
  if err := f(&s); err != nil {
    return err
  }
  if s.Stopped() {
    break
  }
}

where, for example,

f := func(s *iterutil.State) error {
  repl := s.Cur().(*roachpb.RangeDescriptor)
  if something(repl) {
    return errors.New("something is not good!")
  } else if somethingElse(repl) {
    return s.Done() // that's it, won't be called again
  }
  return nil
}

Note how s.Done() returns a nil error to make this pattern nicer, otherwise we'd run the danger of calling s.Done() and then running some more code in the closure like in this buggy code:

if somethingElse(repl) {
  s.Done()
}
// do something else we didn't actually want to do
panic("oops")
return nil

[tbg]

@vrongmeal did the hard part of introducing a better API, but there are a few old style iterations left for him to convert.

[vrongmeal]

yeah! I'm working on it 🙂

[vrongmeal]

I updated the MVCCIterate function in pkg/storage/mvcc.go using iterutil package:

1. In the following line the comment says it should continue but it returns true which means that it's done. Does this require to be fixed?
   https://github.com/cockroachdb/cockroach/blob/71566fd4fa0a4efd579a2fb46c3738f92b5a4dbc/pkg/server/status.go#L368

2. I often found myself writing this:

   cur.Stop()
   return err
   

   Should I add a method StopE like:

   func (c *Cur) StopE(err error) error {
       *c.done = true
       return err
   }
   
   // Stop will be equivalent to `StopE(nil)`:
   func (c *Cur) Stop() error {
       return StopE(nil)
   }
   

   @tbg ^

[tbg]

Yikes! Good catch. Looks like I introduced this bug in two places back in the day: https://github.com/cockroachdb/cockroach/commit/32988c09928bbb971c0bdfd9280e396b5c9d656c

Definitely need to fix.

+1 on StopE as well.

[ajwerner]

Here's my hot take on this issue though I'm happy to be waved away. Returning the booleans from iterator functions is bad due to its ambiguity. Adding a linter to label the boolean is not great for reasons discussed above but it would make the problem less bad. Instead we've added this iterutil package in https://github.com/cockroachdb/cockroach/pull/52158. The problem with that is that it always allocates. When most of these iteration loops were written, they probably also incurred an allocation for the closure. Since go1.13 that's probably no longer the case. Another note is that these type assertions with iterutil are awkward. Another note is that any iteration function that does not already return an error and propagate it is awkward to use (some of them, like in mvcc, do already take a closure that returns a boolean and an error).

The proposal is: ban closures which return a boolean to indicate stopping early. If you want to support stopping early, return a sentinel error that won't be propagated. iterutil.StopIteration = errors.New("stop iteration")? It seems simpler, more explicit, and more less awkward than the whole iterutil thing.

[tbg]

@ajwerner yes, I think that's where we've converged to in #52764. @vrongmeal sorry this is such a windy road. Would you be up for a change of course towards the sentinel error? It looks like it will ultimately be easier to use and understand.

@ajwerner I'm lukewarm about the linter part of this, this seems to be casting too wide a net, but that's a lesser point.

[vrongmeal]

Would you be up for a change of course towards the sentinel error? It looks like it will ultimately be easier to use and understand.

Ofcourse! I'll revert iterutils.State changes and update iterators with the sentinel error approach.