Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ErrDeliveryNotInitialized variable #191

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

quantumsheep
Copy link

@quantumsheep quantumsheep commented Apr 14, 2023

Useful for checking the type of error

@Zerpet
Copy link
Contributor

Zerpet commented Apr 14, 2023

What's the use case for this?

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

@Zerpet Zerpet self-assigned this Apr 14, 2023
@quantumsheep
Copy link
Author

I want to restart my consumer when this specific error happens

@Zerpet
Copy link
Contributor

Zerpet commented Apr 19, 2023

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.

@quantumsheep
Copy link
Author

quantumsheep commented Apr 19, 2023

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

@lukebakken
Copy link
Contributor

@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

@Zerpet
Copy link
Contributor

Zerpet commented Apr 20, 2023

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?

@quantumsheep
Copy link
Author

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?

@Zerpet
Copy link
Contributor

Zerpet commented Apr 24, 2023

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.

@smallhive
Copy link

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

@lukebakken lukebakken force-pushed the expose-err-delivery-not-initialized-variable branch from 758f7aa to b3c445f Compare September 4, 2023 17:55
@lukebakken
Copy link
Contributor

lukebakken commented Sep 4, 2023

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 lukebakken self-assigned this Sep 4, 2023
@smallhive
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants