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

Support clients that don't handle returned messages #30

Closed
victorges opened this issue Jul 13, 2021 · 0 comments · Fixed by #35
Closed

Support clients that don't handle returned messages #30

victorges opened this issue Jul 13, 2021 · 0 comments · Fixed by #35

Comments

@victorges
Copy link
Contributor

victorges commented Jul 13, 2021

Following up on #29

There is a possible issue with the publisher logic due to the fact that it always creates
a NotifyReturn channel around here, but doesn't really know or even have a way to
enforce consumers of the library to actually consume that channel.

By not controlling whether the channel will be really consumed in the end, it is possible
that that background go-routine would get stuck in the exact line referenced above. This
means not only a go-routine possibly leaking in the background, but actually the whole
*amqp.Channel loop hanging as well, since they also have no protection against channels
with no consumers on the other side, as you can see here.

What they do have protection on is that they won't send to a channel that hasn't been
explicitly subscribed with a Notify* function call, so the implicit contract there is that
if you subscribe to a channel, you should read all data sent to it or it should have enough
buffer not to hang the senders (which for cancel/closed channels it can easily be 1 #29).

My suggestion is to do something similar and not create a return channel automatically,
but actually only if the consumer calls some function like Returns() chan Return, setting
up the return channel lazily and only after consumer expressing intent of actually
processing it. That would require some interface changes though like not returning a return
channel from NewPublisher or creating an alternate constructor and updating docs on
the current one to clarify that consumers must listen to the channel if they use mandatory
or immediate publishings.

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 a pull request may close this issue.

1 participant