Skip to content

subscription.close() does not flush pending acks. #2558

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

Closed
lc-chrisbarton opened this issue Aug 25, 2017 · 7 comments
Closed

subscription.close() does not flush pending acks. #2558

lc-chrisbarton opened this issue Aug 25, 2017 · 7 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@lc-chrisbarton
Copy link

lc-chrisbarton commented Aug 25, 2017

#2380

The new api has made my code simpler so thanks. However I find that now there is no ACK which returns a promise then in a Cloud Function I have to write code like this i.e. the call to 'subscription.close()' does not flush the pending ACKs before resolving:

    return new Promise(
      function (resolve, reject) {
        messages.map(message => { message.ack() })
        subscription.close()
          .then(() => {
            setTimeout(function () {
              resolve()
            }, 3000)
          })
          .catch(err => reject(err))
      })
@stephenplusplus
Copy link
Contributor

Thanks for reporting. cc @callmehiphop for help.

@stephenplusplus stephenplusplus added the api: pubsub Issues related to the Pub/Sub API. label Aug 25, 2017
@callmehiphop
Copy link
Contributor

I had originally decided to not add callback/promise support to ack and nack because now we write those requests to a stream and there isn't a reliable way to catch errors. However I think it's probably worth adding support for use cases like this, @stephenplusplus wdyt?

I think if you were to set a timeout to call close after the ack, as opposed to after calling close, that would provide you with a temporary solution.

return new Promise((resolve, reject) => {
  messages.forEach(message => message.ack())
  setTimeout(() => subscription.close().then(resolve, reject), 3000)
})

@lukesneeringer
Copy link
Contributor

Worth noting: acks in Pub/Sub are best effort and Pub/Sub does not reliably tell you if they error. So, error checking will be incomplete at best (which is why we were told to just have .ack return void).

@lukesneeringer
Copy link
Contributor

@callmehiphop That said, subscription closure should just automatically flush pending acks.

@lukesneeringer lukesneeringer changed the title Query: pubsub: api redesign #2380 subscription.close() does not flush pending acks. Aug 25, 2017
@callmehiphop
Copy link
Contributor

callmehiphop commented Aug 25, 2017 via email

@lc-chrisbarton
Copy link
Author

lc-chrisbarton commented Aug 25, 2017 via email

@lukesneeringer
Copy link
Contributor

It currently does, but it sounds like we are maybe hitting a race condition
where the process ends before they can flush completely

Got it. Probably related to the GCF function ending.

@callmehiphop callmehiphop added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

4 participants