Use native promises

[mohd-akram]

No description provided.

[cressie176]

Thanks @mohd-akram, I'll review shortly

[kibertoad]

Where is this coming from?

[mohd-akram]

This does the same as this and was necessary for a test. 'close' is emitted before waitForConfirms is called so it wasn't being handled there. this.pending is set to null when the channel is closed (in Channel._rejectPending) so that's used to check.

[kibertoad]

this sounds like it could result in memory leaks. when do we dispose of unconfirmed?

[mohd-akram]

True, I guess. Ostensibly it's disposed in the 'close' handler in lib/channel.js. However, due to the check in the while loop it exits midway (unconfirmed seems to have values of false). I've updated the code to match what's in the 'close' handler.

[kibertoad]

why make it async?

[mohd-akram]

My understanding of Promise.try is it makes all exceptions, synchronous or asynchronous, to get thrown in .catch. Making the function async does the same thing by wrapping the whole code in a promise.

[cressie176]

I'm confused by how the open was originally being used. There are some cases when then is never called, so the ChannelOpen rpc may execute after the code has continued.

I'm also not sure why Promise.try was required in the first place. ch.allocate can throw a synchronous error, but never does when I run the test suite. If we're uncomfortable with making the function async, then both the following appear to work...

// Decide that Promise.try wasn't necessary to start with
function open(ch) {
  ch.allocate();
  return promisify(function(cb) {
    ch._rpc(defs.ChannelOpen, {outOfBand: ''}, defs.ChannelOpenOk, cb);
  })();
}

// Emulate Promise.try without async
function open(ch) {
  return new Promise((resolve, reject) => {
    try {
      ch.allocate();
      ch._rpc(defs.ChannelOpen, {outOfBand: ''}, defs.ChannelOpenOk, (err, channel) => {
        if (err) return reject(err);
        resolve(channel);
      });
    } catch (err) {
      reject(err);
    }
  })
}

[kibertoad]

async is redundant if we are returning Promise already, which I believe we are

[cressie176]

In the original code if ch.allocate throws an error while still synchronous, it will be thrown rather than rejected. Promise.try prevents this by wrapping the supplied function in a try/catch and rejecting caught errors. That's the only reason for using Promise.try that I can see, but it doesn't seem to be necessary in this case

[mohd-akram]

I've removed the async since it seems unnecessary.

[kibertoad]

Could you also fix conflicts?

[mohd-akram]

Updated.

[cressie176]

all good @kibertoad?

[cressie176]

Published in amqplib@0.10.0

Thank you @mohd-akram