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

Wait for drain when publish/sendToQueue returns false #129

Closed
mattiekat opened this issue May 20, 2020 · 5 comments
Closed

Wait for drain when publish/sendToQueue returns false #129

mattiekat opened this issue May 20, 2020 · 5 comments
Labels

Comments

@mattiekat
Copy link

I am trying to read through the code and I know for a fact I have not seen any waits for the drain event form channel, however, I am not sure if they are needed.

According to the amqplib docs, when publish or sendToQueue returns false, you should wait for the drain event before re-trying. It looks like the code currently just returns false and leaves it to the user to retry; but that requires accessing channelWrapper._channel.on('drain', cb) to do so.

You can find the docs on this at the bottom of this section.

@mattiekat mattiekat changed the title Call drain when publish/sendToQueue returns false Wait for drain when publish/sendToQueue returns false May 20, 2020
@mattiekat mattiekat changed the title Wait for drain when publish/sendToQueue returns false Wait for drain when publish/sendToQueue returns false May 20, 2020
@SimoneDagna
Copy link

SimoneDagna commented Feb 8, 2021

The same goes for return event.
Accordingly with: https://www.rabbitmq.com/publishers.html

When a published message cannot be routed to any queue, and the publisher set the mandatory message property to true, the message will be returned to it. The publisher must have a returned message handler set up in order to handle the return (e.g. by logging an error or retrying with a different exchange)

channelWrapper.on('return', cb) doesn't work.

Directly attaching event listener using ConfirmChannel inside setup callback seems to work but feels wrong.

@navedmirCM
Copy link

navedmirCM commented May 20, 2021

Any update on channel drain event? I can see the drain event being fired but no messages are getting dropped. Not sure, whether it is handled in this library. I can't find the code where drain is handled

@richardklafter
Copy link
Contributor

richardklafter commented Aug 20, 2021

Reading the code if you always await chances are you will never hit the drain issue because this library significantly throttles how quickly you can publish messages. If you just publish 100K+ messages without await you will have a bad day.

//This will work fine because the internal setImmediate call will throttle your publishes to 1 per event loop cycle.
for (let index = 0; index < 500000; index++) {
  await channel.publish(exchange, topic, Buffer.from("yourmessage"+index)
}
//This will bust hard!
for (let index = 0; index < 5000000; index++) {
  channel..publish(exchange, topic, Buffer.from("yourmessage"+index)
}

I might open a PR to fix or I might just purge this library from our code base largely dependent on if its maintainer responds.

@jwalton
Copy link
Owner

jwalton commented Aug 20, 2021

I am more than happy to accept a PR.

github-actions bot pushed a commit that referenced this issue Aug 21, 2021
## [3.2.3](v3.2.2...v3.2.3) (2021-08-21)

### Bug Fixes

* fixed issue with publish ignoring 'drain' event ([e195d9b](e195d9b)), closes [#129](#129)
@jwalton
Copy link
Owner

jwalton commented Aug 21, 2021

🎉 This issue has been resolved in version 3.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

5 participants