feat(disposition): support undeliverable-here in modified outcomes

[mbroadst]

Previously, specifying undeliverable-here as true in a modified
outcome simply resulted in the message being rejected. This patch
adds tracking to the outgoing link management in order to not
redeliver messages to links that indicate that messages are not
deliverable there.

[mbroadst]

@grs here is the amqp10-based test case:

'use strict';
const amqp = require('amqp10'),
      Policy = amqp.Policy,
      expect = require('chai').expect,
      config = { address: 'localhost' };

let test = {};
describe('modified', () => {
  afterEach(() => test.client.disconnect().then(() => { delete test.client; }));

  it('should respect undeliverable here', function(done) {
    test.client = new amqp.Client(Policy.Utils.RenewOnSettle(1, 1, Policy.Default));
    return test.client.connect(config.address)
      .then(() => Promise.all([
        test.client.createSender('test.disposition.queue'),
        test.client.createReceiver('test.disposition.queue')]))
      .spread((sender, receiver) => {
        let received = 0;
        receiver.on('message', msg => {
          expect(received).to.eql(0);
          received++;
          receiver.modify(msg, { undeliverableHere: true });

          return test.client.createReceiver('test.disposition.queue')
            .then(receiver2 => {
              receiver2.on('message', msg => {
                expect(received).to.eql(1);
                done();
              });
            });
        });

        return sender.send('testing');
      });
  });
});

NOTE: this requires a local queue named test.disposition.queue be created.

[grs]

If possible, I'd really like to make the tracking lazy, only activating if actually requested/needed, and only then registering the queue observer and tracking the messages.

[mbroadst]

sure that sounds reasonable. you think its enough to just register it during handle below, with a boolean flag on the class?

(actually maybe just making the SequenceSet and optional would solve the requirement for the boolean, and marginally reduce memory consumption?)

[grs]

Either of those works. One other point is that we need to deregister the observer when the consumer ends.

[mbroadst]

I wondered about that. From other examples of QueueObserver's in the codebase I didn't see anything that explicitly removed itself from the observer list, so assumed (dangerously/lazily 😄) it was cleaned up automatically somewhere.

[mbroadst]

one more question here: is the right place to stop observation in detached?

[grs]

Yes.

[mbroadst]

@grs okay I think that should take care of your comments from yesterday.

PS I @mentioned you in the most recent commit to this repo which breaks the build at least for me locally by duplicating a friend class directive in the main Queue header.

[grs]

Looks good, thanks! Can you attach this as a patch to a JIRA? As qpid-cpp is still in svn I don't think I can merge the PR directly.

[mbroadst]

@grs sure thing, I'll do you one better: https://patch-diff.githubusercontent.com/raw/apache/qpid/pull/9.patch

[grs]

Actually, I've confirmed a patch isn't necessary. I can use your PR branch. I'll give this a test today and all going well merge it in.

[grs]

@mbroadst this is now committed. I somehow messed up the intended merge commit (I think perhaps on git svn rebase), so I'm afraid I neither marked the JIRA for this (https://issues.apache.org/jira/browse/QPID-7324) nor kept your name in the commit log. Thanks very much for the contribution though!

[mbroadst]

@grs no worries, it's the functionality I'm after not the attribution 😄