Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sails prevents all custom signal handlers from running #3693

Closed
kevinburkeshyp opened this Issue Apr 8, 2016 · 17 comments

Comments

5 participants
@kevinburkeshyp
Copy link

kevinburkeshyp commented Apr 8, 2016

It's impossible to do your own cleanup based on incoming signals because Sails registers its own handlers for SIGTERM, SIGINT, and 'exit', which lower Sails and then exit the process. At worst these should be override-able by the user, at best they should check whether other there are other listeners for the same signals.

  var listeners = {
    sigusr2: function() {
      sails.lower(function() {
        process.kill(process.pid, 'SIGUSR2');
      });
    },
    sigint: function() {
      sails.lower(function (){
        process.exit();
      });
    },
    sigterm: function() {
      sails.lower(function (){
        process.exit();
      });
    },
    exit: function() {
      if (!sails._exiting) {
        sails.lower();
      }
    }
  };
@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 8, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

@kevinburkeshyp

This comment has been minimized.

Copy link
Author

kevinburkeshyp commented Apr 8, 2016

You can register a beforeShutdown hook in Sails, but there's no way to tell from there which signal triggered the shutdown hook. The world our process runs in also does not revolve around Sails.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 8, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

@kevinburkeshyp

This comment has been minimized.

Copy link
Author

kevinburkeshyp commented Apr 8, 2016

Latest version.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 8, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

@kevinburkeshyp kevinburkeshyp changed the title sails should not steal listeners sails prevents all custom signal handlers from running Apr 8, 2016

@kevinburkeshyp

This comment has been minimized.

Copy link
Author

kevinburkeshyp commented Apr 8, 2016

Node, NPM versions don't matter. I think Windows uses different signals, so "any Unix" for operating system.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 8, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

kevinburkeshyp pushed a commit to Shyp/sails that referenced this issue Apr 8, 2016

Kevin Burke
Remove bootstrap and signal handlers
App lifting and lowering should be handled entirely in user-land, where the
user can combine lowering with other shutdown tasks. Once we merge this change,
we'll need to update the API to handle SIGTERM and shut itself down. This is
a benefit, since our shutdown handlers will finally actually run.

Removes the `sails.config.bootstrap` option, which introduces unnecessary
callback complexity to Sails and is unused in our codebase.

Fixes balderdashy#3693.

kevinburkeshyp pushed a commit to Shyp/sails that referenced this issue Apr 8, 2016

Kevin Burke
Remove bootstrap and signal handlers
App lifting and lowering should be handled entirely in user-land, where the
user can combine lowering with other shutdown tasks. Once we merge this change,
we'll need to update the API to handle SIGTERM and shut itself down. This is
a benefit, since our shutdown handlers will finally actually run.

Removes the `sails.config.bootstrap` option, which introduces unnecessary
callback complexity to Sails and is unused in our codebase.

Fixes balderdashy#3693.

kevinburkeshyp pushed a commit to Shyp/sails that referenced this issue Apr 8, 2016

Kevin Burke
Remove bootstrap and signal handlers
App lifting and lowering should be handled entirely in user-land, where the
user can combine lowering with other shutdown tasks. Once we merge this change,
we'll need to update the API to handle SIGTERM and shut itself down. This is
a benefit, since our shutdown handlers will finally actually run.

Removes the `sails.config.bootstrap` option, which introduces unnecessary
callback complexity to Sails and is unused in our codebase.

Fixes balderdashy#3693.
@mikermcneil

This comment has been minimized.

Copy link
Member

mikermcneil commented Apr 11, 2016

Hey @kevinburkeshyp, you bring up a good point. If you wouldn't mind putting together proposed usage for configuration to disable these handlers in a proposal, we'll take a look. It'd also help to know more about the use case (i.e. is this a devops or test scenario?)

Thanks!

@kevinburkeshyp

This comment has been minimized.

Copy link
Author

kevinburkeshyp commented Apr 11, 2016

It's our main worker process, we just learned our own signal handlers haven't been running for 14 months. On a worker node, for example, we want to boot Sails/our job queue, then stop processing new jobs when we receive a SIGTERM. Unfortunately we've been processing jobs up until the point that Heroku sends a SIGKILL.

My proposal would be for Sails to not register any sort of SIGTERM/SIGUSR handlers whatsoever. I just booted a Sails app with those lines removed from app/private/initialize.js, sent a SIGTERM to the app and it still shut down just fine. An Express app also shuts down just fine when you send it a SIGTERM. I think the user should control how their application shuts down.

I'm not sure I know of another library or framework that attempts to register SIGTERM handler, to the exclusion of any other signal handlers. At the very least, you could pass the handled signal to lower, and/or avoid calling process.exit.

Furthermore the callback from sails.lower gets passed directly to process.exit; I'm presuming that that value isn't an integer. It's not clear, or documented, how process.exit handles non-integer arguments; I presume it exits, but haven't verified this.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 11, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

@kevinburkeshyp

This comment has been minimized.

Copy link
Author

kevinburkeshyp commented Apr 12, 2016

Whoops, merging the commit in our fork closed this issue. Reopening.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented Apr 12, 2016

Hi @kevinburkeshyp! It looks like you may have removed some required elements from the initial comment template, without which I can't verify that this post meets our contribution guidelines. To re-open this issue, please copy the template from here, paste it at the beginning of your initial comment, and follow the instructions in the text. Then post a new comment (e.g. "ok, fixed!") so that I know to go back and check.

Sorry to be a hassle, but following these instructions ensures that we can help you in the best way possible and keep the Sails project running smoothly.

If you feel this message is in error, or you want to debate the merits of my existence (sniffle), please contact inquiries@treeline.io.

@mikermcneil

This comment has been minimized.

Copy link
Member

mikermcneil commented Apr 14, 2016

@kevinburkeshyp Sorry it's been giving y'all a rough time :/

The reason those process signal handlers came to be is because we wanted a way to send all attempts to exit an application through the same code path in core (this ensures that even if something crazy went wrong with a custom hook, etc, we're sure that the same shutdown logic runs). A related tidbit on that here (Re .close()): expressjs/express#2737 (comment)

I'd definitely be down to get rid of these handlers in core if we can prove via tests that they're not necessary; e.g.:

  • spinning up a sails app as a child process, and sending a bunch of requests to it. Then halfway through that finishing, blasting the child proc with a SIGTERM and verifying that subsequent requests are prevented and that already-initiated requests that are still associated with open TCP connections are immediately destroyed
  • as a child process: generating a new sails app, cding into it, and then lifting in a dev environment w/ the grunt hook enabled. Then make a couple of changes to a large files in assets/ to trigger grunt watch, and before it can finish, send SIGTERM. Afterwards, the grunt grandchild process should no longer be alive. (see #1916)

@kevinburke if you or anyone else reading this disagrees strongly with the expected behavior of those tests, I'm open to discussing different default behavior there longer term, but we need to at least start here for consistency/compatibility. If no one has time to help out by writing those tests right now, we could also expose an optional configuration flag that disables process signal binding altogether. But I agree that if it turns out we can remove these without introducing issues, we should.

P.S. For anyone reading this who forgets what all these various capitalized letters mean as often as I do: earlier this year when I was working on our tests, I jotted down some notes on the various process signals that have since turned into a sort of personal cheat sheet. Here's a link in case you find it helpful.

P.P.S. And @kevinburkeshyp, thanks for taking the time to write up more about your use case-- having that context helps a lot.

@kpturner

This comment has been minimized.

Copy link

kpturner commented Apr 14, 2016

It is a bit difficult to follow because of the sailsbot entries that @kevinburkeshyp expertly ignored on every post :) I hit this problem for a short while and couldn't work out why my handler was not being called, but ultimately it turned out that my handler was unnecessary and I stopped being concerned about it. It would be useful if the handlers are to remain in place to be able to somehow configure user defined functions for them to call before they carry on and do their own thing. At least then we can do some sort of bespoke tidy up if need be.

@mikermcneil

This comment has been minimized.

Copy link
Member

mikermcneil commented Apr 14, 2016

@kpturner that'd probably be the most efficient move-- that way we're not tearing apart everything, we're just exposing a lifecycle callback of sorts (the one caveat is that you obviously can't intercept SIGKILL, but we just wouldn't expose that one).

At this point, I think the thing I'm missing is whether it would be most useful to have the configuration exposed on a per-signal basis or for the teardown in general. My gut says that we'd want to be specific here, since this is really only a consideration when running a production app in its own process (if you're shutting down sails programmatically via sails.lower(), you can always just put any signal-agnostic custom teardown logic in the callback.)

Going with the strategy of a lifecycle callback for a moment, here's a quick proposal if anyone wants to run with it:

// e.g. in config/env/production.js
module.exports = {

  beforeSIGTERM: function (done){
    // If `done` is invoked with something truthy, said output will be logged to the console as an error.
    return done();
  },
  beforeSIGINT: function (done){
    // If `done` is invoked with something truthy, said output will be logged to the console as an error.
    return done();
  }
}

  // Note:
  // would be good to implement case folding for these
  // (e.g. in case you do `beforeSigterm` or `beforesigterm`)
  // If >1 casing is provided, sails should fail to lift w/ an error.
@kpturner

This comment has been minimized.

Copy link

kpturner commented Apr 14, 2016

Yes that sort of thing would have worked for my "use case". Obviously I can't say if it would be OK for the situation that @kevinburkeshyp faced though.

@sailsbot

This comment has been minimized.

Copy link

sailsbot commented May 26, 2016

@kevinburkeshyp,@sailsbot,@mikermcneil,@kpturner: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.