JS Exception in server should be logged in app.log

springmeyer commented 9 years ago

I've found that currently if throw error happens in the server:

nothing is logged
the server exits
then atom-shell client exits by responding to the server exit here

So a server crash basically silently and immediately brings the whole app down.

One step forward (and what this pull request is limited to) would be to at least log the cause of the crash to app.log so that users can help us know what happened and fix it. A server crash should never happen, but obviously if it does then we want to know the full stack trace to be able to fix it.

/cc @bsudekum can you ensure this same situation is logged in tilemill (I think it already is).

/cc @yhahn and @camilleanne for gut check

Next action before merging:

[ ] A test to simulate a server crash, and then confirm the stack trace shows up in the logs (no idea yet how to do this)

While I'm here thinking about this, a blue sky solution would be:

the error is logged
the server, upon exit (or somehow before) communicates the error to the shell
the shell knows a fatal error was hit, catches it, and displays a modal for the user
the user is 1) ask to report a bug, and 2) told that the error can be found later on in the logs at ~/.mapbox-studio/app.log
only one button for the modal exists, which is exit
when exit is clicked the shell shuts down

bsudekum commented 9 years ago

@springmeyer nice, this is great. I also just added something similar but for error events in tilemill/atom-shell, see here.

Do you think both stderr and uncaughtException data events are needed?

springmeyer commented 9 years ago

Do you think both stderr and uncaughtException data events are needed?

@bsudekum - I'm not sure - Can you test and let me know what you find?

springmeyer commented 9 years ago

bump @bsudekum - let me know what you think on stderr.

wilhelmberg commented 9 years ago

Before continuing here, you might consider merging my PR https://github.com/mapbox/mapbox-studio/pull/1265

The current solution in master is not really async aware, which showed when I activated shelllog(), which seems to take considerably more time on Windows. Consequence was, even though the server process had started successfully it never loaded in atom-shell.

Also atom.quit() should be used instead of process.exit() to allow for a proper shutdown of atom-shell.

wilhelmberg commented 9 years ago

Manually added https://github.com/mapbox/mapbox-studio/commit/3eacc33c09a328452cde3b0f832d00e499ceceb7

mapbox / mapbox-studio-classic

JS Exception in server should be logged in app.log #1294