-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Add a top-level exception handler to the Node process #7521
Comments
I'm actually wondering if we already have code somewhere that tries to restart the Node process if it dies - it seems like we have some logic around auto-reconnecting domains, so it seems like that's an expected case. But @jadbox was running into a problem where an extension was basically making all the Node features in Brackets unusable because it threw an exception in the Node code, and it seemed like the whole process died and didn't restart. |
Marking high priority since we should really look at this soon. |
Not sure this is really bigger than a breadbox, but I'm going to nominate it on the Kanban board anyway so we don't lose sight of it. |
This is done in brackets-shell. On Windows: https://github.com/adobe/brackets-shell/blob/master/appshell/appshell_node_process_win.cpp#L110 |
Hm, ok. We should try to figure out why it seemed like @jadbox's node process was persistently dead. |
From my experiences with brackets-git, I don't think the node process actually restarts when it's brought down by an uncaught exception. I've recently had an issue with this and file watchers just stayed off until I restarted my Brackets. |
Forgot to note that the behavior was different on Linux and Windows - on Windows file watchers "survived" the launch of a non-existing file with an error from node in the console, but on Linux they crashed. I had to do a This is the domain file brackets-git are using currently: https://github.com/zaggino/brackets-git/blob/master/src/domains/cli.js |
Also, when there's a typo - nothing will help ... I've submitted a PR which helps with this a bit: adobe/brackets-shell#432 |
This is definitely necessary. I'm getting crashes about every ~5-10 minutes with Brackets open - at first I thought it was an extension, but it persisted after I disabled all of them. The app goes blue, none of the menu options work and it won't close unless I terminate the process manually. Really annoying because besides this issue I absolutely adore Brackets. |
@chronister on which platform do you see this issue? I've merged adobe/brackets-shell#432 to get a better idea what might have caused the issue to see some output in the console. But it's definitely only a small step towards a solution for this issue. |
@ingorichter Windows 7 64 Bit. Since I posted that, it's actually been fairly stable - I'll have to mess around a bit more to see if I can reproduce the conditions that cause instability; I think it might be something in a node.js project I had open. However, opening the developer tools doesn't seem to be working - the chrome window says it's unable to connect to localhost:9234. Also, building the latest copy of brackets-shell is proving difficult, as I'm getting some jasmine errors. I'll try to see what I can do. |
Nominate for 1.0. |
@chronister What issues do you see building brackets-shell? Do you see them on master? |
That symptom is a different problem - it's a crash in the shell's render process. An exception in the Node process doesn't take down the Brackets UI - it just disables some functionality like file watching and extension installation. Could you file a separate bug on what you're seeing, and list the extensions you have installed if any? Thanks. On Apr 16, 2014, at 8:32 PM, "Andrew" <[email protected]mailto:[email protected]> wrote: This is definitely necessary. I'm getting crashes about every ~5-10 minutes with Brackets open - at first I thought it was an extension, but it persisted after I disabled all of them. The app goes blue, none of the menu options work and it won't close unless I terminate the process manually. Really annoying because besides this issue I absolutely adore Brackets. Reply to this email directly or view it on GitHubhttps://github.com//issues/7521#issuecomment-40677489. |
Reviewed, keeping for 1.0 |
@dangoor Any objections to pushing this out of 1.0? It hasn't seemed like a common problem yet. In 1.0-review we removed the milestone -- but for this one we wanted to make sure it's ok with you too. |
Yeah, I agree that it hasn't seemed a common problem and we can punt from 1.0. |
Should this be closed as 'move to backlog' since a card was created for it? https://trello.com/c/DsAfNtlA/1254-s-research-gracefully-handle-node-exceptions |
Yes, that makes sense. Closing move to back log... |
We should globally catch exceptions in Node, so the entire process doesn't get hosed by a bad extension. If we do catch such an exception, we probably need to restart the node process, but at least it wouldn't stay in a bad state.
The text was updated successfully, but these errors were encountered: