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

RabbitMQ publish doesn't take advantage of amqp-connection-manager's reliable publish #673

Closed
ttshivers opened this issue Jan 15, 2024 · 2 comments
Labels

Comments

@ttshivers
Copy link
Contributor

The rabbitmq package's publish implementation calls publish on a raw ConfirmChannel instead of the ChannelWrapper provided by amqp-connection-manager.

Since it's not using the ChannelWrapper, it's missing out on all the reliability features that amqp-connection-manager provides for publishing messages.

Was this an intentional decision to not use the ChannelWrapper ?

return new Promise((resolve, reject) => {
this._channel.publish(
exchange,
routingKey,
buffer,
options,
(err, ok) => {
if (err) {
reject(err);
} else {
resolve(ok);
}
}
);
});
}

Instead of using this._channel there, this._managedChannel could be used instead which is the ChannelWrapper

ttshivers added a commit to ttshivers/nestjs that referenced this issue Jan 15, 2024
Publish messages using the ChannelWrapper instead of the raw ConfirmChannel to take advantage of the
reliability features of  `amqp-connection-manager`.

BREAKING CHANGE: This changes the behavior of throwing connection related errors

re golevelup#673
@underfisk
Copy link
Contributor

@ttshivers If you feel confident about this change and can provide test coverage to assert that it works as intended I'll be happy to review and merge.
I don't know what the reason was behind not to use ChannelWrapper but from your explanation it seems logical to use so

ttshivers added a commit to ttshivers/nestjs that referenced this issue Jan 16, 2024
Publish messages using the ChannelWrapper instead of the raw ConfirmChannel to take advantage of the
reliability features of  `amqp-connection-manager`.

BREAKING CHANGE: This changes the behavior of throwing connection related errors

re golevelup#673
ttshivers added a commit to ttshivers/nestjs that referenced this issue Jan 16, 2024
Publish messages using the ChannelWrapper instead of the raw ConfirmChannel to take advantage of the
reliability features of  `amqp-connection-manager`.

BREAKING CHANGE: This changes the behavior of throwing connection related errors

re golevelup#673
underfisk pushed a commit that referenced this issue Jan 16, 2024
Publish messages using the ChannelWrapper instead of the raw ConfirmChannel to take advantage of the
reliability features of  `amqp-connection-manager`.

BREAKING CHANGE: This changes the behavior of throwing connection related errors

re #673
@ttshivers
Copy link
Contributor Author

Fixed by #678

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

2 participants