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

Topic.flush() returns immediately, before all items are published #1620

Closed
feywind opened this issue Sep 9, 2022 · 5 comments · Fixed by #1636
Closed

Topic.flush() returns immediately, before all items are published #1620

feywind opened this issue Sep 9, 2022 · 5 comments · Fixed by #1636
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@feywind
Copy link
Collaborator

feywind commented Sep 9, 2022

Topic.flush() is promisified, so it returns either a Promise or takes a callback, depending on how it's called. This calls Publisher.flush(), which likewise can deal with Promises. However, MessageQueue.publish(), which Publisher.flush() calls, does not support Promises, only callbacks. But Publisher.flush() is expecting it to support Promises, so no callback is passed down, and Publisher.flush() gets back undefined, which passes through as a resolve, and no wait happens.

@feywind feywind added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: pubsub Issues related to the googleapis/nodejs-pubsub API. labels Sep 9, 2022
@feywind feywind self-assigned this Sep 9, 2022
@feywind
Copy link
Collaborator Author

feywind commented Sep 9, 2022

@baeminbo
Copy link

A public issue tracker is created for this issue: https://issuetracker.google.com/issues/246207643

@feywind
Copy link
Collaborator Author

feywind commented Sep 12, 2022

Actually, a related issue: #1463

@feywind
Copy link
Collaborator Author

feywind commented Sep 21, 2022

I think I was looking at the wrong branch or something, because this does actually promisify the calls: https://github.com/googleapis/nodejs-pubsub/blob/main/src/publisher/index.ts#L113

That was fixed by this PR: e494fb7

...which would've been in release 2.9.0.

So I need to re-study the original problem and see if there's something else.

gcf-merge-on-green bot pushed a commit that referenced this issue Sep 22, 2022
It's possible to end up in a situation where more publishes are pending when publish() is called, but they aren't actually waited for. This adds 'drain' event support to the main publisher queue and waits for 'drain' on all queues before calling it done.

Maybe fixes #1620 🦕
@feywind
Copy link
Collaborator Author

feywind commented Sep 22, 2022

I found some possibilities. Please give this a try and see if it does what's needed, and if not, I'll reopen.

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 googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants