Pubsub force shutdown if shutdown isn't working. #5294

christhompsongoogle · 2022-12-02T03:02:38Z

Pubsub force shutdown if shutdown isn't working.

codecov-commenter · 2022-12-02T03:10:21Z

Codecov Report

Base: 56.34% // Head: 56.32% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (4fe8eb5) compared to base (dfea3a5).
Patch coverage: 25.00% of modified lines in pull request are covered.

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

Impacted Files	Coverage Δ
src/emulator/pubsubEmulator.ts	`13.25% <25.00%> (+1.41%)`	⬆️
src/emulator/auth/state.ts	`84.87% <0.00%> (-0.57%)`	⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

t Please enter the commit message for your changes. Lines starting

src/emulator/pubsubEmulator.ts

bkendall · 2022-12-08T18:39:25Z

src/emulator/pubsubEmulator.ts

+      await downloadableEmulators.stop(Emulators.PUBSUB);
+    } catch (e: unknown) {
+      this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e));
+      exec(PUBSUB_KILL_COMMAND, (err, stdout) => {


Yeah, here - if we do this on Windows, are issues silently logged and it moves on?

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.

…bsubHack

bkendall · 2022-12-21T22:03:43Z

src/emulator/pubsubEmulator.ts

+      await downloadableEmulators.stop(Emulators.PUBSUB);
+    } catch (e: unknown) {
+      this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e));
+      if (process.platform === "win32") {


This check looks like the opposite of what you wanted. Do you want !==?

Correct thanks for pointing it out

bkendall · 2022-12-22T17:02:53Z

src/emulator/pubsubEmulator.ts

+    } catch (e: unknown) {
+      this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(e));
+      if (process.platform !== "win32") {
+        exec(PUBSUB_KILL_COMMAND, (err, stdout) => {


I just noticed: exec isn't synchronous, should this be wrapped in a Promise and awaited, or use execSync?

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.

stfsy · 2022-12-27T13:24:18Z

@christhompsongoogle having this PR merged would be very, very handy :) Can I support somehow?

christhompsongoogle · 2022-12-29T00:08:54Z

@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.

bkendall · 2023-01-04T17:16:48Z

src/emulator/pubsubEmulator.ts

+    } 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`.


This is fine, but it might be cleaner to use execSync

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.

bkendall · 2023-01-04T17:17:13Z

src/emulator/pubsubEmulator.ts

+            if (stdout) {
+              this.logger.logLabeled("DEBUG", "pubsub", JSON.stringify(stdout));
+            }
+            resolve(null);


Passing null isn't necessary.

Suggested change

resolve(null);

resolve();

bgazzera · 2023-10-06T15:05:46Z

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 ps aux works in Alpine (it reverses the pid & user columns in the output).

When terminating the pubsub emulator you will see a message like:

Stopping Pub/Sub Emulator 
sh: invalid number 'root'
sh: invalid number 'root'

A solution is to install the procps package, like so: apk --no-cache add procps

Pubsub force shutdown if shutdown isn't working.

c2e06cd

christhompsongoogle added 6 commits December 5, 2022 18:13

idk

e378c26

fix

03db576

fix

affffac

revert readme

67df4d0

t Please enter the commit message for your changes. Lines starting

changelog

8b8a89d

fix

0d8541c

christhompsongoogle requested a review from bkendall December 6, 2022 02:25

Merge branch 'master' into pubsubHack

6ae703b

christhompsongoogle assigned bkendall Dec 6, 2022

Merge branch 'master' into pubsubHack

f0c8d1b

christhompsongoogle enabled auto-merge (squash) December 6, 2022 18:40

bkendall reviewed Dec 8, 2022

View reviewed changes

christhompsongoogle added 3 commits December 20, 2022 13:55

Add guard for windows.

29b9a89

Merge branch 'master' into pubsubHack

0ec63cb

Merge remote-tracking branch 'refs/remotes/origin/pubsubHack' into pu…

d3186ae

…bsubHack

bkendall requested changes Dec 21, 2022

View reviewed changes

Fix

30b8179

christhompsongoogle requested a review from bkendall December 22, 2022 00:03

bkendall requested changes Dec 22, 2022

View reviewed changes

Wrap exec in a promise

3ea6d0a

christhompsongoogle requested a review from bkendall January 4, 2023 01:04

bkendall approved these changes Jan 4, 2023

View reviewed changes

bkendall disabled auto-merge January 4, 2023 17:17

christhompsongoogle added 2 commits January 4, 2023 15:35

Merge branch 'master' into pubsubHack

fd7784f

Switch to execSync

4fe8eb5

christhompsongoogle enabled auto-merge (squash) January 5, 2023 00:06

christhompsongoogle merged commit ffb09f8 into master Jan 5, 2023

christhompsongoogle mentioned this pull request Jan 6, 2023

pubsub: Unable to terminate process #5252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pubsub force shutdown if shutdown isn't working. #5294

Pubsub force shutdown if shutdown isn't working. #5294

christhompsongoogle commented Dec 2, 2022

codecov-commenter commented Dec 2, 2022 •

edited

bkendall Dec 8, 2022

christhompsongoogle Dec 20, 2022 •

edited

bkendall Dec 21, 2022

christhompsongoogle Dec 22, 2022

bkendall Dec 22, 2022

christhompsongoogle Dec 29, 2022

stfsy commented Dec 27, 2022

christhompsongoogle commented Dec 29, 2022

bkendall Jan 4, 2023

christhompsongoogle Jan 5, 2023

bkendall Jan 4, 2023

bgazzera commented Oct 6, 2023

Pubsub force shutdown if shutdown isn't working. #5294

Pubsub force shutdown if shutdown isn't working. #5294

Conversation

christhompsongoogle commented Dec 2, 2022

codecov-commenter commented Dec 2, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

christhompsongoogle Dec 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stfsy commented Dec 27, 2022

christhompsongoogle commented Dec 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgazzera commented Oct 6, 2023

codecov-commenter commented Dec 2, 2022 •

edited

christhompsongoogle Dec 20, 2022 •

edited