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

Don't allow publishing to an exchange that is not open. #402

Merged

Conversation

v-yarotsky
Copy link
Contributor

Prevents nasty issues like callbacks that are not fired when publisher confirmations are turned on, caused by broker-client acknowledgement sequencing mismatch.

Whenever a connection error occurs, if publisher confirmations are enabled, all currently unacked messages emit 'ack error' instead of not calling the callback altogether.

There also are two tests that don't pass for me on master, as well as on this branch.

test/test-connection-blocked.js:
assert.js:93
  throw new assert.AssertionError({
        ^
AssertionError: 1 == 0
    at process.<anonymous> (/Users/vlad/Projects/node-amqp/test/test-connection-blocked.js:58:12)
    at process.emit (events.js:95:17)
FAIL
test/test-flow.js:
/Users/vlad/Projects/node-amqp/node_modules/longjohn/dist/longjohn.js:195
        throw e;
              ^
Error: NOT_IMPLEMENTED - active=false
    at Connection._onMethod (/Users/vlad/Projects/node-amqp/lib/connection.js:518:15)
    at self.parser.onMethod (/Users/vlad/Projects/node-amqp/lib/connection.js:136:12)
    at AMQPParser._parseMethodFrame (/Users/vlad/Projects/node-amqp/lib/parser.js:366:10)
    at frameEnd (/Users/vlad/Projects/node-amqp/lib/parser.js:93:16)
    at frame (/Users/vlad/Projects/node-amqp/lib/parser.js:78:14)
    at header (/Users/vlad/Projects/node-amqp/lib/parser.js:64:14)
    at AMQPParser.execute (/Users/vlad/Projects/node-amqp/lib/parser.js:137:21)
    at Connection.<anonymous> (/Users/vlad/Projects/node-amqp/lib/connection.js:174:21)
    at Connection.emit (events.js:95:17)
---------------------------------------------
    at module.exports.run (/Users/vlad/Projects/node-amqp/test/harness.js:49:23)
    at Object.<anonymous> (/Users/vlad/Projects/node-amqp/test/test-flow.js:1:84)
    at Module._compile (module.js:456:26)
    at Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Module._load (module.js:312:12)
    at Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
FAIL

Vlad Yarotsky added 2 commits August 7, 2015 13:22
Also, ack errors all unacked messages on connection errors in addition
to channel errors.
When the connection goes down, it will only be "detected" during the
next attempt to interact with it. If this happens to be a basicPublish,
then it would fire a callback if not for the fix.
@@ -25,7 +25,7 @@ conn.once('ready', function () {
later(function(){
// This will emit a NOT_FOUND error b/c we're no longer bound to the exchange
var thrown = false;
conn.addListener('error', function(e){
exchange.addListener('error', function(e){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some help on this one: the error event is not triggered on connection for some reason. That is not the case if I don't add error handlers on channelOpenOk.

@v-yarotsky v-yarotsky force-pushed the vy-fix-confirm-sequencing-after-recovery branch from 5755654 to c153988 Compare August 7, 2015 23:23
@v-yarotsky
Copy link
Contributor Author

Might help with #373.

postwait added a commit that referenced this pull request Aug 8, 2015
…r-recovery

Don't allow publishing to an exchange that is not open.
@postwait postwait merged commit 7a42179 into postwait:master Aug 8, 2015
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