Export on exit #2415

michi88 · 2020-07-01T02:54:45Z

Description

It was a bit harder as I expected because we didn't have a clean shutdown from the spawned java processes. It seems something with gRPC is not working well because the java processes already start stopping on the 'Ctrl-c' SIGINT before we actually try to stop then in the graceful shutdown. Added detached: true for that and they all seem to shutdown correctly now. Maybe fixable on the Java side of things as well, this issue might be related: grpc/grpc-java#6512

I also did make this command work:

$ firebase emulators:start --import ./.dataDirThatDoesNotExistYet --export-on-exit
// when the --import [dir] does not exist this would be equivalent of running
$ firebase emulators:start --export-on-exit ./.dataDirThatDoesNotExistYet

// on exit now that folder exists and this a second time running 
$ firebase emulators:start --import ./.dataDirThatDoesNotExistYet --export-on-exit
// would be equivalent of 
$ firebase emulators:start --import ./.dataDirThatDoesNotExistYet --export-on-exit ./.dataDirThatDoesNotExistYet

The reason I did that is that it makes it a lot easier for first time repo users (only 1 command to start fresh / restart). Else one would need to start, export and start again. Which is just a pain in my opinion.

I've added the options to src/commands/ext-dev-emulators-start.ts and src/commands/ext-dev-emulators-exec.ts as well but I do not understand what they do.

Scenarios Tested

See tests as well

Sample Commands

$ firebase emulators:start --import ./.data --export-on-exit

$ firebase emulators:start --import ./.data --export-on-exit ./data

$ firebase emulators:start --import ./.other --export-on-exit ./data

// fail
$ firebase emulators:start --export-on-exit

// exec works as well
$ firebase emulators:exec --import ./.data --export-on-exit

…T possible

googlebot · 2020-07-01T02:54:52Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

michi88 · 2020-07-01T03:00:59Z

@googlebot I signed it!

googlebot · 2020-07-01T03:01:14Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

samtstern

Overall this is looking great, thank you for doing it so quickly! Just a few questions.

samtstern · 2020-07-01T12:24:11Z

src/emulator/commandUtils.ts

+ */
+export function setExportOnExitOptions(options: any): any {
+  if (options.exportOnExit) {
+    if (typeof options.import === "string") {


You can assume everything in options (or at least all the flags) are strings when they're defined, no need to do all this typeof checking.

If you're just using typeof to check if it exists, we prefer checking the truthiness instead. So for example you have this:

options.exportOnExit = typeof options.exportOnExit === "string" ? options.exportOnExit : options.import;

I think you can just do:

options.exportOnExit = options.exportOnExit || options.import;

options.exportOnExit can be a boolean when used without a [dir]. Like --import ./data --export-on-exit

Ah yes good point! Want to just add a comment about this? Also seems like typeof options.import checks are still not necessary.

Agree it is not the prettiest, added some test cases and a note.

samtstern · 2020-07-01T12:28:39Z

src/emulator/commandUtils.ts

+    let receivedSIGINTS = 0;
+    process.on("SIGINT", async () => {
+      try {
+        receivedSIGINTS = receivedSIGINTS + 1;


If I send SIGINT twice won't this logic run await onExit(options) and await controller.cleanShutdown() more than once?

onExit will run only once, cleanShutdown() twice and then gives up. Current behaviour is that cleanShutdown() is always called every time you do SIGINT. Not ok when cleanShutdown is hanging on some stop and never finishes or take ages. You need to be able to bail out there.

I was considering changing it to process.once("SIGINT", ... and only do everything once but then I was thinking of the case that an onExit takes a long time and the user want to just quit, in that case we still want to do the cleanShutdown().

Let me know what you think.

Ok so after fighting my Windows PC for a few hours I finally got everything running. The new SIGINT handling seems fine however:

When I do a single Ctrl+C I now get warnings like Firestore Emulator has exited upon receiving signal: SIGINT which make it seem like something went wrong but I think that's normal?

The log message Received SIGINT 1 is confusing because it feels like 1 is the signal code but it's the count. Suggest Received SIGINT (count=1)

Hmmmm actually while playing with this I quickly got into a state where the Database emulator is running free and holding port 9000, so I have to shut it down with pkill or similar. That's not good.

src/emulator/commandUtils.ts

src/emulator/controller.ts

src/emulator/downloadableEmulators.ts

samtstern · 2020-07-01T12:34:18Z

Oh and @michi88 please add an entry to CHANGELOG.md

michi88 · 2020-07-01T14:30:13Z

@samtstern added it to the changelog. Out of curiosity. How do you guys handle the release versioning / changelog / documentation?

samtstern · 2020-07-01T14:37:25Z

@michi88 our release process pretty much works like this:

We use master as the development branch for the next release and it's always kept green. Every PR goes to master and includes a CHANGELOG entry if it should be included in release notes.
Once in a while a human on the team says "hey should we do a release?" at which point we read over the changelog and decide on the SemVer implications. Then we kick off a Cloud Build job that does the release.
The CHANGELOG is directly converted into the GitHub release notes and then we clear it out and start over.

…tring or a bool

samtstern · 2020-07-01T16:41:57Z

@michi88 I did a bunch of testing and all the import/export stuff works great! My remaining concerns are about the SIGINT handling and the newly detached processes. I don't think that's working quite right.

michi88 · 2020-07-01T16:49:50Z

Yeah, it would be best if we can handle the SIGINT in the java children correctly (wait for exit). I think the RPC process goes down to quickly although the jvm is still running. Best would really be to have them attached. I did try all kinds of combinations for the stdio settings, to no avail. In the end I think they should be attached but the SIGINT signal should not propagate to the child.

It is very probable this is fixable in the java side of things. This has been merged end of last year, do you guys have that in?
grpc/grpc-java#6512

samtstern · 2020-07-01T18:13:34Z

@michi88 looking at the source code of the emulator we do call server.awaitTerminated() so I don't actually think that gRPC issue is the root of the problem? I am a bit stumped here to be honest,

samtstern · 2020-07-01T18:53:34Z

@michi88 I want to add that the bugs this has revealed are not your fault, but they are blockers at the moment. So I'm gonna see if I can get some more of my teammates to look into this and offer ideas.

yuchenshi · 2020-07-01T21:58:03Z

My two cents on SIGINT:

When you Ctrl-C, a SIGINT is sent to ALL processes in the foreground group. In our case, that includes the Emulator Java processes. There is no way you can "catch" that signal in the CLI process to prevent it from reaching the child process.

If we want to shield the java process from receiving SIGINT, we need to modify the CLI to launch the java process in the background. In Node.js, I believe it can be achieved via spawn(..., ..., {detached: true}).

Disclaimer: Not an expert on this domain and haven't tried that.

michi88 · 2020-07-02T10:50:20Z

Here some code to play around with this: https://gist.github.com/michi88/48e1b3b8530163b45ce1db75aa35451e

In the end I think it is best to not run in detached mode but ignore the SIGINT signal in the emulators (children). One could argue we only want to ignore the first SIGINT and do our clean shutdown but on a second do go into a quick shutdown.

yuchenshi · 2020-07-06T17:25:37Z

@michi88 Thanks for the suggestion! However, modifying the Firestore emulator to ignore the first SIGINT is really complicated on our side, since the Firestore emulator is also included the Google Cloud CLI (gcloud) and we need to keep the backwards compatibility there. What issue do you see with detached mode? Let's see if we can address that instead.

samtstern · 2020-07-07T14:37:37Z

@yuchenshi while manually testing this I quickly got into a situation where I had exited the parent process but the detached emulator processes were still running and holding on to their ports. I'm not 100% sure how I got there, I was doing a lot of manual testing with Ctrl+C at different times.

So I think we all agree that detached: true is the way to go but we need to make absolutely sure we never exit without killing all the children.

michi88 · 2020-07-07T14:38:34Z

EDIT: didn't see your comment above...

@samtstern What were the issue you were observing with detached mode?

One tradeoff we have is:

Do we always wait for a clean shutdown of the emulators no matter what, this makes sure we never leave orphan emulator processes running
OR
Do we skip the clean shutdown after a third SIGINT for example (like we do now) ensuring that we do give back the shell to the user when they really want to (but potentially leve emulators running).

Also, we now only listen for SIGINT, we probably want to change that to also listen for SIGTERM, SIGHUP and SIGQUIT.

I would propose we do allow for a non-clean exit on repeated exit signals (with the warning we're not cleanly shutting down) and we also start listening for the other 3 exit signals.

michi88 · 2020-07-07T14:41:53Z

@samtstern I could see if we can make it such that we always wait for a clean shutdown if you'd like.

samtstern · 2020-07-07T14:42:13Z

@michi88 I do agree that as a developer you want to get control of your terminal back when you slam Ctrl+C multiple times. But it can definitely be hard to find the right zombie processes to kill after.

I think we know all of the PIDs of the child processes. Before doing a non-clean exit could we print a warning like:

Warning: you have forced the Emulator Suite to exit without waiting for {n} subprocesses to finish. These processes may still be running on your machine.  To force them to exit run:

  - kill {pid 1}
  - kill {pid 2}

michi88 · 2020-07-07T14:43:34Z

Works for me as well, we do have the pid. Let me know your decision.

samtstern · 2020-07-07T14:58:05Z

@michi88 let's go with the warning message, thanks for being so flexible! We'll need to make sure we show a platform-dependent warning; kill for Mac/Linux and TaskKill for Windows.

- Listen for more stop signals - Stop running on fatal start of emulator - Stop emulators on exit that are 'starting' - Faster shutdown (parallel emulator stops)

michi88 · 2020-07-10T17:54:39Z

Ok, so changed quite some things here to make it all work together:

Listen for more stop signals
Stop running on fatal start of emulator, for example try: JAVA_HOME=/does/not/exist firebase emulators:start
Stop emulators on exit that are 'starting'
Faster shutdown (parallel emulator stops)
Throw an error when non existing emulator is used, for example: firebase emulators:start --only doesnotexist

This fixes a lot of cases that emulators keep running and shutdown is a bit faster as well.

Looking forward to your feedback.

samtstern

Wow this is a really great PR that cleans up a lot of edge cases! Thank you for sticking with it. I have one final comment, but approved otherwise.

samtstern · 2020-07-10T18:03:11Z

src/emulator/controller.ts

+        fs
+          .readFileSync(
+            path.join(path.resolve(options.import), HubExport.METADATA_FILE_NAME),
+            "utf8"


Did you mean to handle this else case or log something? Having it empty seems strange.

I indeed wanted to add a warning there. Added now!

samtstern · 2020-07-10T20:28:36Z

Oh and don't worry about the CLA bot, I checked internally and you definitely signed it so I will force-merge this when ready.

…rupted auto export)

michi88 · 2020-07-11T12:58:54Z

Oh and don't worry about the CLA bot, I checked internally and you definitely signed it so I will force-merge this when ready.

Not sure what you're seeing but it has been looking ok from my side for a while already:

Wow this is a really great PR that cleans up a lot of edge cases! Thank you for sticking with it. I have one final comment, but approved otherwise.

Awesome! I did have some fun diving into this project!

samtstern · 2020-07-11T13:39:12Z

@michi88 merged!

quantuminformation · 2020-07-14T06:47:21Z

Will it be in the next release?

Nice work, and not even a Google staff.

samtstern · 2020-07-14T07:00:42Z

@quantuminformation yep, whenever a change is merged into master it gets included with the next release.

anantanandgupta · 2020-08-01T19:08:07Z

i am getting my emulator stopping before it can export the database. this happens on precc of ctrl+c on macos with [email protected]

^C
i  emulators: Received SIGINT (Ctrl-C) for the first time. Starting a clean shutdown.
i  emulators: Please wait for a clean shutdown or send the SIGINT (Ctrl-C) signal again to stop right now.
i  Automatically exporting data using --export-on-exit "./rtdb" please wait for the export to finish...

⚠  emulators: Received SIGINT (Ctrl-C) 2 times. You have forced the Emulator Suite to exit without waiting for 1 subprocess to finish. These processes may still be running on your machine:

┌───────────────────┬────────────────┬───────┐
│ Emulator          │ Host:Port      │ PID   │
├───────────────────┼────────────────┼───────┤
│ Database Emulator │ localhost:9000 │ 20288 │
└───────────────────┴────────────────┴───────┘

To force them to exit run:

kill 20288

below is the command i used to start the process

firebase emulators:start --only functions,hosting,database --inspect-functions --import ./rtdb --export-on-exit

can anyone please suggest a way to gracefully stop the process and wait for the database to be exported.

samtstern · 2020-08-03T10:41:33Z

@anantanandgupta this looks like #2507 and I have a PR coming to fix it very shortly.

anantanandgupta · 2020-08-03T11:07:15Z

thanks @samtstern ... will wait for this. one more point to make over here, which no one has pointed out is that emulator ui is also not stopped. it keeps on getting new port even after we kill that pid shown in the message. on a fresh start emulator ui gets port 4000. but after this issue and killing the pid (or even sending sigint) it keeps emulator ui running, thus keeping the port. next execution it gets 4001 ... 4002 and onwards. i have to kill the nodejs process manually to free that port.

alexandermckay · 2020-08-11T00:56:50Z

@samtstern works perfectly. Thank you very much!

michi88 added 3 commits July 1, 2020 02:39

spawn processes in detached mode to make a graceful shutdown on SIGIN…

70bd611

…T possible

Adds --export-on-exit flag to automatically export emulator data on exit

eb9bd30

remove console.log

18bf246

googlebot added the cla: no label Jul 1, 2020

Merge branch 'master' into export-on-exit

920d71a

googlebot added cla: yes Manual indication that this has passed CLA. and removed cla: no labels Jul 1, 2020

samtstern reviewed Jul 1, 2020

View reviewed changes

samtstern self-assigned this Jul 1, 2020

michi88 added 2 commits July 1, 2020 16:16

mutate --export-on-exit options in Command.before

2a2736b

add --export-on-exit flag to CHANGELOG.md

1d307a4

michi88 added 2 commits July 1, 2020 17:08

check for options.exportOnExit instead of overriding options.force

7df2b04

more test cases and note about that options.exportOnExit being be a s…

d8b832b

…tring or a bool

Merge branch 'master' into export-on-exit

04c13c7

michi88 added 2 commits July 10, 2020 19:25

Merge branch 'master' into export-on-exit

01e049e

More robust starting and stopping of emulators:

3423dc3

- Listen for more stop signals - Stop running on fatal start of emulator - Stop emulators on exit that are 'starting' - Faster shutdown (parallel emulator stops)

samtstern approved these changes Jul 10, 2020

View reviewed changes

samtstern mentioned this pull request Jul 10, 2020

Update test timeout for timezone test #2443

Merged

add warning when we can't find the import metadata file (due to inter…

4a9cea2

…rupted auto export)

samtstern merged commit 053be81 into firebase:master Jul 11, 2020

yuchenshi mentioned this pull request Jul 21, 2020

Emulators are not correctly terminated when firebase emulators:exec subprocess is killed #2477

Closed

This was referenced Jan 14, 2021

No graceful exit using NPM Scripts #2507

Closed

No graceful exit using NPM Scripts that trigger multiple SIGINT #3034

Closed

Export on exit #2415

Export on exit #2415

Uh oh!

Conversation

michi88 commented Jul 1, 2020

Description

Scenarios Tested

Sample Commands

Uh oh!

googlebot commented Jul 1, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

michi88 commented Jul 1, 2020

Uh oh!

googlebot commented Jul 1, 2020

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samtstern commented Jul 1, 2020

Uh oh!

michi88 commented Jul 1, 2020

Uh oh!

samtstern commented Jul 1, 2020

Uh oh!

samtstern commented Jul 1, 2020

Uh oh!

michi88 commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern commented Jul 1, 2020

Uh oh!

samtstern commented Jul 1, 2020

Uh oh!

yuchenshi commented Jul 1, 2020

Uh oh!

michi88 commented Jul 2, 2020

Uh oh!

yuchenshi commented Jul 6, 2020

Uh oh!

samtstern commented Jul 7, 2020

Uh oh!

michi88 commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michi88 commented Jul 7, 2020

Uh oh!

samtstern commented Jul 7, 2020

Uh oh!

michi88 commented Jul 7, 2020

Uh oh!

samtstern commented Jul 7, 2020

Uh oh!

michi88 commented Jul 10, 2020

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

michi88 commented Jul 1, 2020 •

edited

Loading

michi88 commented Jul 7, 2020 •

edited

Loading