Pubsub force shutdown if shutdown isn't working. #5294
Conversation
Codecov ReportBase: 56.34% // Head: 56.32% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #5294 +/- ##
==========================================
- Coverage 56.34% 56.32% -0.02%
==========================================
Files 315 315
Lines 21288 21295 +7
Branches 4340 4341 +1
==========================================
Hits 11994 11994
- Misses 8255 8261 +6
- Partials 1039 1040 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/emulator/pubsubEmulator.ts
Outdated
| await downloadableEmulators.stop(Emulators.PUBSUB); | ||
| } catch (e: unknown) { | ||
| this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e)); | ||
| exec(PUBSUB_KILL_COMMAND, (err, stdout) => { |
There was a problem hiding this comment.
Yeah, here - if we do this on Windows, are issues silently logged and it moves on?
There was a problem hiding this comment.
Added a windows check and will follow-up if we see any windows issues (and test once I have a machine to play with). This moves the logging point but there's nothing to do at this moment since we're already shutting down.
src/emulator/pubsubEmulator.ts
Outdated
| await downloadableEmulators.stop(Emulators.PUBSUB); | ||
| } catch (e: unknown) { | ||
| this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e)); | ||
| if (process.platform === "win32") { |
There was a problem hiding this comment.
This check looks like the opposite of what you wanted. Do you want !==?
There was a problem hiding this comment.
Correct thanks for pointing it out
src/emulator/pubsubEmulator.ts
Outdated
| } catch (e: unknown) { | ||
| this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e)); | ||
| if (process.platform !== "win32") { | ||
| exec(PUBSUB_KILL_COMMAND, (err, stdout) => { |
There was a problem hiding this comment.
I just noticed: exec isn't synchronous, should this be wrapped in a Promise and awaited, or use execSync?
There was a problem hiding this comment.
This can be a fire-and-forget so long as Node doesn't terminate before executing it. In that case I think we'll just be missing the output. At any rate I think wrapping in a promise like this should do the trick.
|
@christhompsongoogle having this PR merged would be very, very handy :) Can I support somehow? |
Sorry I've been a little distracted on this one. I'll have this in by Jan 6 once our release process has exited the holiday freeze. |
src/emulator/pubsubEmulator.ts
Outdated
| } catch (e: unknown) { | ||
| this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e)); | ||
| if (process.platform !== "win32") { | ||
| // The exec is wrapped in a promise so we can block on it's completion via `await`. |
There was a problem hiding this comment.
This is fine, but it might be cleaner to use execSync
There was a problem hiding this comment.
I don't love this buffer paradigm but it is shorter/cleaner - I added it for now and the kill-command part works so I'll revisit if we encounter any additional issues.
src/emulator/pubsubEmulator.ts
Outdated
| if (stdout) { | ||
| this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(stdout)); | ||
| } | ||
| resolve(null); |
There was a problem hiding this comment.
Passing null isn't necessary.
| resolve(null); | |
| resolve(); |
|
For anyone coming at this issue when using an Alpine-based linux distro (most Docker images), the fix won't work because of the way When terminating the pubsub emulator you will see a message like: A solution is to install the procps package, like so: |
Pubsub force shutdown if shutdown isn't working.