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

Error pending confirmation callbacks on channel close #498

Merged
merged 2 commits into from
Apr 10, 2019

Conversation

johanneswuerbach
Copy link
Contributor

Otherwise waitForConfirms never terminates or terminates node

Additional this includes a commit 099673e, which makes it possible to install this module directly from git (instead of the published package), but I'm happy to drop this if preferred.

This allows the package to be installed directly from git
@squaremo
Copy link
Collaborator

This makes sense. It is a behaviour change though -- people may not be prepared for rejections from waitForConfirms (though I hope they would be). I guess we can call that out in release notes when it comes to it.

The tests are failing because of some ES6 syntax which snuck in channel.js L36 (honestly, if I had the time I'd rewrite in ES6 and babel it for older runtimes ..).

@johanneswuerbach
Copy link
Contributor Author

While I understand the behavior change I think the current behavior is worse as messages won’t be acknowledged (the channel is closed) so the promise will never resolve and leave the process just hanging, which seems more like a bug.

Afaik you already need to handle the reject case anyway as messages could be rejected for other reasons, not?

Will fix the es6 syntax, which node versions are currently supported?

@squaremo
Copy link
Collaborator

Afaik you already need to handle the reject case anyway as messages could be rejected for other reasons, not?

Absolutely -- in this case the risk is people relying too much on the "happy path", so I'm not too troubled by it.

Will fix the es6 syntax, which node versions are currently supported?

All the way back to Node 0.8. (!)

Otherwise waitForConfirms never terminates or terminates node
@johanneswuerbach
Copy link
Contributor Author

@squaremo tests fixed.

@johanneswuerbach
Copy link
Contributor Author

johanneswuerbach commented Apr 2, 2019

@squaremo friendly ping :-)

@johanneswuerbach
Copy link
Contributor Author

Maybe @cressie176? Sorry for the spam, but installing this dependency from git requires curl, which is a bit of a bummer :-(

@squaremo squaremo merged commit 773eb92 into amqp-node:master Apr 10, 2019
@johanneswuerbach
Copy link
Contributor Author

@squaremo would you also be able to have a look at #503?

@squaremo
Copy link
Collaborator

Sorry @johanneswuerbach, I don't always get a lot of time to react to PRs and issues -- but at least you've made it easy for me by explaining the motivation and keeping the change simple

@johanneswuerbach
Copy link
Contributor Author

johanneswuerbach commented Apr 10, 2019

@squaremo sure, no worries. As an open source maintainer myself I completely understand, let me know if there is anything I can do to help.

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.

2 participants