Resolve subscription.close() once streams have been flushed.

[STeveShary]

After using the pub/sub client for test, I have found that I wanted to know when the queue was flushed before exiting. The comment in the code states that a promise is returned when no callback is provided: https://github.com/googleapis/nodejs-pubsub/blob/master/src/subscription.js#L377 but the code does not return a promise.

Environment details

• OS: OSX
• Node.js version: 8.6.0
• npm version: 5.3.0
• @google-cloud/pubsub version: 0.16.2

Steps to reproduce

1. Open a subscription
2. Close a subscription with no callback.
3. Execute .then() on the promise from the subscription close method.

Expected: the callback in the .then() is executed.
Actual: Error calling .then on undefined.

I have completed a fix with a test here: STeveShary@2bf9c33 . Your contribution guide doesn't recommend a PR yet, but I can create one quickly.

[callmehiphop]

@STeveShary thanks for reporting! I'm sorry you're running into this issue. Generally we do not return promises directly in any of our methods. Instead we use a decorator to wrap them.

I attempted to reproduce the error you were seeing, however in my local testing close() returned a promise for me. If you wouldn't mind putting together a minimal example of how to reproduce this error, I think that would help try and resolve this.

[STeveShary]

@callmehiphop Thanks for responding. My commit from above: STeveShary@2bf9c33 has a really simple test as part of it. I found that I had to add the code fix to get the test to pass.

[STeveShary]

I threw a PR over to help with that.

[callmehiphop]

@STeveShary we stub the decorators in our unit tests, so promises will not work there unless we return the promise directly, which I think we want to avoid so we can stay uniform with the rest of the code.

[callmehiphop]

I created the following e2e test in /system-test/pubsub.js

it.only('should return a promise', function() {
  var subscription = topic.subscription(generateSubName(), {
    maxConnections: 1,
  });

  subscription
    .on('error', err => console.error(err))
    .on('message', message => {});

  return subscription.close().then(function() {
    console.log('it works!');
  });
});

And it passes/logs as expected. By any chance have you been exclusively testing in the unit test files? That would have the potential for a false positive.

[STeveShary]

Hmmm... I know we ran into this problem initially when we were trying to close and make sure the queue was flushed before exiting. Let me look...

[STeveShary]

Ok. Jogged the memory. The issue that we ran into is that we found that our acks were not being sent back. This we found was because there is no command that will return a promise that tells us when the queue for acks has been drained. We did see in the close() method:

  this.flushQueues_().then(function() {
    self.closeConnection_(callback);
  });

This would be GREAT! But, alas it does not return that promise and therefore we can't know when the queue is clear other than setTimeout(..., 1010); (which is not optimal)

For our tests, we want to send acks for all subscriptions but our tests almost everytime exit before the acks are flushed from the queue. Making close() return a promise allows us to know that exactly from the its fulfillment.

[callmehiphop]

Ah, I see. In most cases that queue isn't actually used. Its primary function is to act as a backup in case we're unable to make a streaming connection. I'd need to investigate a bit more, but it might be possible to implement a flush (or similar) event to listen to.

[stephenplusplus]

@callmehiphop do we need these changes: #52?

[callmehiphop]

No, it can be closed.

…

On Feb 10, 2018 7:59 AM, "Stephen" ***@***.***> wrote: @callmehiphop <https://github.com/callmehiphop> do we need these changes: #52 <#52>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#50 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABoNAxVqMbBIlgXK-lvizYdwim4tSqQQks5tTZK4gaJpZM4Rtg2I> .

[callmehiphop]

@STeveShary we've got a fix prepared for this (#92) if you have the bandwidth to test it out that would be fantastic! Otherwise we should have it released within the next day or two.