proposal: async.AckFunc should not return an error

[kaperys]

I propose that we amend the async.AckFunc method signature (https://github.com/uw-labs/substrate-tools/blob/master/async/source.go#L36) such that it no longer returns an error itself, but instead causes the parent async.ConsumeMessages function to return. This change would simplify execution path of the provided async.ConsumerMessageHandler and allow the user to simply defer ack(), should they wish.

At the time of writing the current implementation only returns a non-nil error when the context passed to async.ConsumerMessageHandler is cancelled.

A consideration: the current implementation provides immediate feedback should the context be cancelled and therefore the ack not be processed, which allows the user to rollback any transactions made during execution of their message handler code. Implementing this change would mean that feedback would only be provided after the message handler code returns.

Related discussion: #6 (review)

[heedson]

When introducing this, I treated this a lot like a file.Close error where if it's deferred (as is a reasonable standard), the error wouldn't affect the function as it's already returned and out of scope. The reason the error return was kept was purely for the common ctx.Done being accompanied by the ctx.Err. As you said, though, it will never error for another reason (the user can't and shouldn't know this, though).

And this may have a different solution but something doesn't sit well with me if we call an ack func and the ack can potentially not happen but no feedback is given (hence the ctx cancel being reported). The reason the ctx is in the ackFunc at all is so that user code would never reach the point where the ackFunc could hang forever due to it interacting with a channel.

Just some raw-ish thoughts to get some context (ctx heh) in the current design choices here.

[povilasv]

file.Close error where if it's deferred (as is a reasonable standard), the error wouldn't affect the function as it's already returned and out of scope.

the defer close idiom for file.Close is actually not a good idea for -> https://www.joeshaw.org/dont-defer-close-on-writable-files/

I'm big fan of pubsub ConsumeMessages interface https://github.com/utilitywarehouse/go-pubsub/blob/master/pubsub.go#L60, if ack function wouldn't return error, maybe we don't need ack function and just ack on the users behalf once consumer handler function returns. I feel like:

func consumerHandler() {
do stuff
ack()
do more stuff
return
}

is not a valid use case. As if do more stuff blocks ,it might block consumption of other messages from the queue, altough technically acked.

[heedson]

I think your link for the Close is a good source, so thanks!

I specifically created this package to solve the "not a valid case" case so I'm against removing it. The current async/sync in substrate handles that for you if you don't care about acking. This was to abstract away the channel usage while still giving the user agency to ack when they want, which is what this exposes.

[povilasv]

out of curiosity what's the usecase for it? have you used this feature anywhere?

[heedson]

It was to ack a message during a long running task without causing a resend of the event due to a timeout passing (we were using nats streaming). So, we didn't want to receive the event again if the timeout passes and so we do only the critical work before the ack (recording we've consumed from the queue). Then, the longer task is run after the ack, which can be rerun if it fails, due to us recording that we've consumed the event (but it hasn't completed successfully).

To be totally transparent, we stopped working on the branch around the time this was added as a feature (due to priority shifts), but it's an example of where the functionality was imagined.

From a different angle, perhaps my vision of the benefits of async vs sync isn't necessarily correct but if we auto ack for the user after running their handler, are we not just making a parallel sync option (which makes it async)? A part of the power behind the async interface in substrate, in my view, was that the user was in control of when and where to ack for a message.

One thing I thought about was having the struct in this package expose an endpoint for both - one handler that accepts a flexible ackFunc and one that does that for you but runs the sync approach in parallel. What do you, and others, think about this?

[povilasv]

It was to ack a message during a long running task without causing a resend of the event due to a timeout passing (we were using nats streaming). So, we didn't want to receive the event again if the timeout passes and so we do only the critical work before the ack (recording we've consumed from the queue). Then, the longer task is run after the ack, which can be rerun if it fails, due to us recording that we've consumed the event (but it hasn't completed successfully).

You could do:

do a thing
go func() do another thing
ack()

but I guess you will start consuming more messages then and it might not be better than

do a thing
ack()
do another thing
return // and unblock the pipe to allow more messages to be consumed

so all good I agree with you, valid use case.

A part of the power behind the async interface in substrate, in my view, was that the user was in control of when and where to ack for a message.

I guess, as I never had the use case and ❤️ the simplicity with pubsub.

[povilasv]

One thing I thought about was having the struct in this package expose an endpoint for both - one handler that accepts a flexible ackFunc and one that does that for you but runs the sync approach in parallel. What do you, and others, think about this?

I think we can make another handler, maybe make the other handler a different package? As it's a bit different API and feels like a different package to me.

[heedson]

I don't mind that - we could maybe see what we want the packages to end up as, since async squats a quite good package name. My reasoning for perhaps having both on the same handler is similar to API that have Read and ReadAll - one is a bit raw and the other is a bit more whole-package, but both technically use the same underlying logic, just a little differently. I don't feel strongly about this, just that it looked like both initialised cleanly from the same options, but I guess having both methods means shenanigans could happen with the channels etc...

[povilasv]

Looking at Go

ReadAll is in a different package ioutil,https://golang.org/pkg/io/ioutil/#ReadAll
Read in io as Reader interface ;)
so they are in different packages.

e both initialised cleanly from the same options, but I guess having both methods means shenanigans could happen with the channels etc...

that's what I'm afraid of one user registering both consumer handlers, etc. More functions more complexity, and the features become less orthogonal, more complicated etc.

[heedson]

I guess that Go only has the Reader interface! 🙂
My example was thinking more about bytes buffers and it's many read and write methods:
https://golang.org/pkg/bytes/#Buffer

I used a bad initial example, oops!

Anyway, limited examples aside, I think registering both consumers sounds dangerous so I'm tending to not liking both on the same one as much as previously.