Defer errors until Flush to avoid incomplete batches

[nickgarlis]

Any create/update/delete operation that returns a validation or marshalling error can leave the message batch in an incomplete state due to short-circuiting. This can result in either:

• Non-atomic transactions if Flush is called (incomplete batch)
• Users being unable to clear the incomplete batch (no API exposed)

This change ensures that errors are collected and deferred until Flush. Instead of returning immediately, the following methods now append errors to a slice checked at Flush:

• AddSet
• DelRule
• SetAddElements

See: #323

[NoobsEnslaver]

Users being unable to clear the incomplete batch (no API exposed)

just close connection without flush?

[nickgarlis]

just close connection without flush?

Yeah that could be an option but IIUC, that's not something we want to do when the connection is lasting.

[NoobsEnslaver]

what is the value of making the connection lifetime as long as possible? If the goal is to save resources, then we are wasting resources by continuing to work with the connection until the next Flush, although we could already know that an error has occurred and we need to stop doing unnecessary work. Also, long live connections may lead to leaks.
In any case, the biggest problem is, as I said, the complication of debugging - you don't know which command had an error, you only know what's in the batch, and so that it's at least somewhat diagnosable, you start Flushing frequently and lose resources again.
I would also like to see more detailed, collected errors during Flush, but not by rejecting pre-validation of commands arguments.

[nickgarlis]

Thanks for explaining.

what is the value of making the connection lifetime as long as possible? If the goal is to save resources, then we are wasting resources by continuing to work with the connection until the next Flush, although we could already know that an error has occurred and we need to stop doing unnecessary work.

IIUC, your argument is that since memory is already being used while the message accumulates, keeping the connection open doesn’t really save much.

That makes sense, but I still think there are cases where you’d want bigger, frequent messages (e.g. a daemon updating geo ipsets). In that situation, why force a connection close on validation failure?

Also, long live connections may lead to leaks.

Do you have any examples of such leaks ? Are they caused by bugs in the underlying libraries ?

In any case, the biggest problem is, as I said, the complication of debugging - you don't know which command had an error, you only know what's in the batch, and so that it's at least somewhat diagnosable

Would something like this actually make debugging a lot worse? Errors could be made richer and carry identifiers for the failing objects:

err := conn.Flush()
if err != nil {
  if validationErr, ok := err.(ValidationError); ok {
      fmt.Println("ValidationError:", validationErr)
  } else if marshallingErr, ok := err.(MarshallingError); ok {
      fmt.Println("MarshallingError:", marshallingErr)
  } else if errno, ok := err.(syscall.Errno); ok {
      fmt.Println("syscall.Errno:", errno)
  } else {
      fmt.Println("Unknown error:", err)
  }
}

My argument is that your message is only really complete when you're ready to Flush. For example an anonymous set that is not tied to a rule would result in a kernel EINVAL but you have no way of spotting that during AddSet. I also think that this approach is more transactional.

This is not an entirely new pattern either. It seems that stateful objects are also implemented in a similar way. See

nftables/obj.go

Line 114 in 207a463

cc.setErr(err)

I would also like to see more detailed, collected errors during Flush, but not by rejecting pre-validation of commands arguments.

AFAIU, there is no way of doing this without adding some kind of validation on Flush() which puts you back in the same spot. Using the anonymous set example again, you’d still end up with a Flush error that references a set somewhere far up in the message.