apostrophecms / apostrophe-monitor

Monitors and restarts an Apostrophe app when your code, templates, etc. change. Like nodemon but much faster because it takes advantage of how Apostrophe works.
8 stars 1 forks source link

Enable monitor to keep running in case Apostrophe can't start #7

Closed joeinnes closed 4 years ago

joeinnes commented 4 years ago

Current behaviour: if Apostrophe can't start (eg: syntax error in a module's index.js), monitor exits to the terminal.

This PR allows monitor to continue running in case Apostrophe doesn't start, and instead runs a basic HTTP server which will return the error message and stack dump, until the error is fixed and Apostrophe can run successfully again.

joeinnes commented 4 years ago

I think I'm registering the callback on line 112, which eventually calls the afterInit callback on line 117.

The grace period of 0 I think just starts closing sockets immediately.

I believe it would work like this:

  1. monitor starts, creates an http server, but doesn't listen anywhere
  2. monitor starts Apostrophe
  3. Error causes Apostrophe to exit
  4. monitor starts the http server listening on the appropriate port
  5. User fixes the error which caused Apostrophe to exit
  6. Changes cause monitor to try to start Apostrophe again
  7. Apostrophe loads all modules successfully, and finishes initialising
  8. Apostrophe triggers the afterInit function
  9. monitor instructs the http server to close all connections immediately
  10. Once all connections close, the callback from the .stop() method fires - at this point, the port should be free to use
  11. This fires the callback provided to the afterInit() function
  12. Apostrophe gets on with its usual business.

What would be the benefit of reusing the listening socket compared to Apostrophe just serving a stack trace on error, regardless of monitor?

boutell commented 4 years ago

Missed that. Seems kosher to me.

Apostrophe can't catch all errors. There's no such thing as async try/catch in old school callback driven code. One of the reasons why A3 is async/await everywhere and uses the apiRoutes feature heavily.

On Thu, Apr 23, 2020 at 4:45 PM Joe Innes notifications@github.com wrote:

I think I'm registering the callback on line 112 https://github.com/joeinnes/apostrophe-monitor/blob/13a60eda079f715d494d6ed632f61bbf9de2f957/monitor.js#L112, which eventually calls the afterInit callback on line 117 https://github.com/joeinnes/apostrophe-monitor/blob/13a60eda079f715d494d6ed632f61bbf9de2f957/monitor.js#L117 .

The grace period of 0 I think just starts closing sockets immediately.

I believe it would work like this:

  1. monitor starts, creates an http server, but doesn't listen anywhere
  2. monitor starts Apostrophe
  3. Error causes Apostrophe to exit
  4. monitor starts the http server listening on the appropriate port
  5. User fixes the error which caused Apostrophe to exit
  6. Apostrophe triggers the afterInit function
  7. monitor instructs the http server to close all connections immediately
  8. Once all connections close, the callback from the .stop() method fires - at this point, the port should be free to use
  9. This fires the callback provided to the afterInit() function
  10. Apostrophe gets on with its usual business.

What would be the benefit of reusing the listening socket compared to Apostrophe just serving a stack trace on error, regardless of monitor?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-monitor/pull/7#issuecomment-618659038, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27LTIBKTIQEHIMGSBMTROCSF5ANCNFSM4MPF3RMQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

joeinnes commented 4 years ago

Yes, I saw that. I am not sure how to set the initFailed function from inside monitor though - it seems that it requires app.js, which in turn requires apostrophe with the options object.

I can't see a way to get a callback defined in monitor onto the Apostrophe's initFailed option until after it's too late and Apostrophe has already loaded (and/or crashed).

boutell commented 4 years ago

Yeah, good point, we can't do that without changing the pattern by which one uses apostrophe-monitor. Good enough for now (and then some)!

On Fri, Apr 24, 2020 at 1:12 PM Joe Innes notifications@github.com wrote:

Yes, I saw that. I am not sure how to set the initFailed function from inside monitor though - it seems that it requires app.js, which in turn requires apostrophe with the options object.

I can't see a way to get a callback defined in monitor onto the Apostrophe's initFailed option until after it's too late and Apostrophe has already loaded (and/or crashed).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-monitor/pull/7#issuecomment-619138505, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27PV6CYCWH3KDGAPCODROHB63ANCNFSM4MPF3RMQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 4 years ago

Thanks! This will be in the next release.

On Fri, Apr 24, 2020 at 1:51 PM Tom Boutell tom@apostrophecms.com wrote:

Yeah, good point, we can't do that without changing the pattern by which one uses apostrophe-monitor. Good enough for now (and then some)!

On Fri, Apr 24, 2020 at 1:12 PM Joe Innes notifications@github.com wrote:

Yes, I saw that. I am not sure how to set the initFailed function from inside monitor though - it seems that it requires app.js, which in turn requires apostrophe with the options object.

I can't see a way to get a callback defined in monitor onto the Apostrophe's initFailed option until after it's too late and Apostrophe has already loaded (and/or crashed).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-monitor/pull/7#issuecomment-619138505, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27PV6CYCWH3KDGAPCODROHB63ANCNFSM4MPF3RMQ .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his