amqp091-go icon indicating copy to clipboard operation
amqp091-go copied to clipboard

Expose `ErrDeliveryNotInitialized` variable

Open quantumsheep opened this issue 1 year ago • 11 comments

Useful for checking the type of error

quantumsheep avatar Apr 14 '23 09:04 quantumsheep

What's the use case for this?

In other words, what would your application do differently if the error matches ErrDeliveryNotInitialized ?

Zerpet avatar Apr 14 '23 10:04 Zerpet

I want to restart my consumer when this specific error happens

quantumsheep avatar Apr 17 '23 23:04 quantumsheep

Don't you want to restart your consumer when any error happens anyway?

In other words, what makes this error different from the app point of view?

Edit: I'm hesitant to modify the public API, unless there's a compelling use case for it.

Zerpet avatar Apr 19 '23 08:04 Zerpet

ErrDeliveryNotInitialized can happen when acknowledging a message for example. I don't want to restart my connection if the error has nothing to do with it

quantumsheep avatar Apr 19 '23 13:04 quantumsheep

@quantumsheep have you actually seen errDeliveryNotInitialized? Is there a way to demonstrate it?

If we're going to change one error, we may want to consider the other private ones:

C:\Users\bakkenl\development\rabbitmq\amqp091-go [main ≡]
> git grep '\<err[A-Z]'
connection.go:465:              return errInvalidTypeAssertion
delivery.go:13:var errDeliveryNotInitialized = errors.New("delivery not initialized")
delivery.go:125:                return errDeliveryNotInitialized
delivery.go:145:                return errDeliveryNotInitialized
delivery.go:170:                return errDeliveryNotInitialized
read.go:438:var errHeartbeatPayload = errors.New("Heartbeats should not have a payload")
read.go:446:            return nil, errHeartbeatPayload
types.go:68:    errInvalidTypeAssertion = &Error{Code: InternalError, Reason: "type assertion unsuccessful", Server: false, Recover: true}
uri.go:16:var errURIScheme = errors.New("AMQP scheme must be either 'amqp://' or 'amqps://'")
uri.go:17:var errURIWhitespace = errors.New("URI must not contain whitespace")
uri.go:74:              return builder, errURIWhitespace
uri.go:87:              return builder, errURIScheme

lukebakken avatar Apr 19 '23 18:04 lukebakken

ErrDeliveryNotInitialized can happen when acknowledging a message for example. I don't want to restart my connection if the error as nothing to do with it

IIRC, RabbitMQ will close the channel if there's any error returned from the server. In this case, the error is coming from the library, and as you are correctly pointing out, you have to restart your consumer (i.e. close-open a new channel). In either case, the channel gets closed. A potential improvement for your app would be to close the channel and open a new one. I agree there's no need to close the connection if there's an error in the ack.

To be honest, I'll be surprised if Delivery.Acknowledger is ever nil. Have you found a case where this happens?

Zerpet avatar Apr 20 '23 18:04 Zerpet

To be honest, I'll be surprised if Delivery.Acknowledger is ever nil. Have you found a case where this happens?

I don't think so, the only error I got when using .Ack is ErrDeliveryNotInitialized.


If I sums up, I would need to restart my consumer whenever an error occurs, whatever the error is?

quantumsheep avatar Apr 21 '23 09:04 quantumsheep

Interesting 🤔 I'm happy to expose this error if we understand why it happens, and document it. We have to add a go-doc comment on the error, describing what it means.

If I sums up, I would need to restart my consumer whenever an error occurs, whatever the error is?

From this library point of view, closing the channel is sufficient to "cancel" a pending message ack. If your consumer app can perform this operation, there is no need to restart the entire consumer app.

Zerpet avatar Apr 24 '23 13:04 Zerpet

Hi guys! I have the same problem and want to have errDeliveryNotInitialized exposed too. What stops us from merging this PR?

I have this issue with Ack. Don't have steps to reproduce, but in my case, I have this during debugging. When receiving a message from a rabbit and going step by step in debug. After that sometimes see thousands messages in log with error

smallhive avatar Aug 28 '23 14:08 smallhive

What stops us from merging this PR?

Reviewing and testing this PR is on my to-do list, but I have other work to finish first.

@smallhive also, please read the comments here. We can't just merge this PR without considering the implications to the public API. We'd like to know why this error is necessary versus what is already raised.

lukebakken avatar Sep 04 '23 17:09 lukebakken

I've read the comments, and from my point of view, the error required for clean understanding is the next: the error means the channel is closed, any other Acking leads to the same result, you must reconnect to the rabbit.

I am trying to reproduce or find a way to reproduce the error. One I can say right now, it happens during debugging when you go through your code step-by-step. I am keeping my investigation

smallhive avatar Sep 05 '23 03:09 smallhive