nats-io / nats-architecture-and-design

Architecture and Design Docs
Apache License 2.0
177 stars 20 forks source link

Message Ack Behavior #72

Closed scottf closed 2 years ago

scottf commented 2 years ago

Overview

This issue is to discuss and then define message ack behavior. Currently the behavior differs between clients. We should unify on a behavior.

Repeat Acks

Options for repeat acks.

  1. When a messages is ack'ed other than progress (+WPI), the ack is recorded alongside the message. If the user tries to ack again, an error is returned.
  2. When a messages is ack'ed other than progress (+WPI), the ack is recorded alongside the message. If the user tries to ack again, the client just ignores this.
  3. No checks are made, the user can ack as much as possible. The server does whatever it does with duplicate acks.

Also to be considered is what if the Ack Policy is Ack None? Should errors be returned or should ack be ignored?

Auto Ack

Options for auto ack. For subscription that supports auto ack, what should happen if the user acks (other than progress) during it's handling. Reminder the client performs auto ack after the user has seen the message.

  1. Allow the ack (n/a for progress (+WPI)), and re-ack during the auto ack phase
  2. Allow the ack (n/a for progress (+WPI)), and skip the auto ack phase
  3. Surface an error during the user ack (prevent the user from acking other than in progress)
  4. Surface an error during the auto ack phase if the user has ack'd

Clients and Tools

Other Tasks

Client authors please update with your progress. If you open issues in your own repositories as a result of this request, please link them to this one by pasting the issue URL in a comment or main issue description.

kozlovic commented 2 years ago

The C client is doing 2., that is, the ack call will be a no-op and return NATS_OK (no error). I could see why we would want to return an error though, so that could be changed in the C client (the behavior is not documented in the docs, so could be changed and considered a bug).

For ack policy "Ack none", the library could simply mark the message as ack'ed (or some other internal state) before being given to the user: depending on the choice made above, the user trying to Ack such message would either get an error (although in this case a more relevant error would be better than "already ack'ed") or be a no-op.

For auto-ack, we can't prevent user from doing "in progress" (or NAck!) , but other than that, we could either have the library mark the message as already ack'ed before giving it to the user - so that again, depending on the choice made above - the user doing an Ack inside the callback would return an error or being a no-op, and then the library can force the ack. For both auto-ack and ack node, looks like having different internal state would be better so that a meaningful message can be returned based on the context (AckNode and AutoAck would have different error messages?).

scottf commented 2 years ago

Ok just want to call something out for clarification on Auto Ack. Does the client

  1. auto ack before handing the message to the user
  2. auto ack after handing the message to the user.

I though the case that makes sense is that we give the message to the user first, so they can handle it, do in progress as necessary, throw an error themselves in which case we would not auto ack.

kozlovic commented 2 years ago

The callback should auto-ack after the user code returns. What I meant is that the library could "mark" the message before giving it to the user if we want the user to not be able to ack.

Note that the "in progress" makes sense only while the user is in the callback. As soon as the user callback returns, the library MUST ack the message.

The case of Java where user can throw an error should still not disable the library from ack'ing the message. You should have a try/catch/finally there to ensure that the library acks the message. If user wants more control, the user should use manual ack.

scottf commented 2 years ago

So the suggestion is to prevent the user from acking if they have chosen auto ack. This is one of the options at the top

aricart commented 2 years ago

Also - the return of an error is language specific, so in some languages it would likely throw. I think this simply needs to return a boolean if it worked, and false if it was ignored. No reason to throw an error (in Go that behaviour works out of the box, because the error is not raised but returned).

aricart commented 2 years ago

Surface an error during the user ack (prevent the user from acking other than in progress)

This also is creating specific conditions where the code is matched to the consumer/etc. If the client callback acks the message or something else, the auto-ack with the previous behaviour (noop) didn't have to worry about it. It just didn't do anything. Complexifying here means that now these things need to be in aligned more, but it is not clear what the benefit is.

scottf commented 2 years ago

@ColinSullivan1 @kozlovic @wallyqs @aricart @jnmoyne @tbeets @variadico I'm looking for votes. My votes.

Repeat Ack: Option 3 is my first choice, option 2 is my second Auto Ack: Option 2 is my choice.

aricart commented 2 years ago

Now the only issue here is that if the ack none (is there even a reply subject provided for those messages).

kozlovic commented 2 years ago

Repeat Ack: Option 2, so that library does not send extra ACK (even though server would correctly handle, we save on traffic) Auto Ack: Option 2, again, why ack twice.

aricart commented 2 years ago

Exactly as Ivan is saying it. ☝️

tbeets commented 2 years ago

Me too.

Repeat Ack: Option 2* Auto Ack: Option 2

scottf commented 2 years ago

Closing. Documented in ADR-15, see PR 76 [change] ADR-15 ack handling