grunt.fail will stop the watch task with the nospawn option #58

Closed
shama opened this Issue Feb 28, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@shama
Member

shama commented Feb 28, 2013

Opening this issue because I know somebody else will soon. :)

grunt.fail will stop the watch task if the nospawn option is enabled. In Grunt v0.3 we had a specialized warnAlternate so the watch task could re-run itself upon failure. Not in v0.4 but even then it didn't always catch all failures. This was one of the reasons the watch moved towards spawning as child processes.

A user can prevent this by setting --force but that isn't a good idea (and a really bad idea for the watch task to set for a user). Some APIs use --force to enable dangerous parts, such as --force will enable grunt.file.delete to remove files outside the cwd and potentially destroy a user's system.

So the question is, do we:

  • Ask for specialized handling of failures in grunt.fail?
  • If nospawn enabled, have the watch duck punch grunt.fail?
  • Listen to process events to try to rerun the watch task where necessary?
  • Just tell users to enable --force?
  • Some other solution?
@joeybaker

This comment has been minimized.

Show comment
Hide comment
@joeybaker

joeybaker Mar 22, 2013

Contributor

Ask for specialized handling of failures in grunt.fail?

Gets my vote. (see: gruntjs/grunt#706)

If nospawn enabled, have the watch duck punch grunt.fail?

Second best option IMHO.

Listen to process events to try to rerun the watch task where necessary?

Ooof. Possible, but slow and error-prone.

  • One of the huge downsides of spawing a sub-process is the ~500ms delay. A major reason to enable nospawn is to avoid that delay, this would introduce a similar, but related slowness.
  • It's possible that while the process is spinning backup it would miss changes.
  • Might be possible that there's initialization done before the watch process that you wouldn't want run twice? Maybe?

Just tell users to enable --force?

That's my current solution with grunt.option('force', !grunt.option('force')), but:

  • It doesn't work if a grunt.fail is called
  • It's super dirty.
Contributor

joeybaker commented Mar 22, 2013

Ask for specialized handling of failures in grunt.fail?

Gets my vote. (see: gruntjs/grunt#706)

If nospawn enabled, have the watch duck punch grunt.fail?

Second best option IMHO.

Listen to process events to try to rerun the watch task where necessary?

Ooof. Possible, but slow and error-prone.

  • One of the huge downsides of spawing a sub-process is the ~500ms delay. A major reason to enable nospawn is to avoid that delay, this would introduce a similar, but related slowness.
  • It's possible that while the process is spinning backup it would miss changes.
  • Might be possible that there's initialization done before the watch process that you wouldn't want run twice? Maybe?

Just tell users to enable --force?

That's my current solution with grunt.option('force', !grunt.option('force')), but:

  • It doesn't work if a grunt.fail is called
  • It's super dirty.
@shama

This comment has been minimized.

Show comment
Hide comment
@shama

shama Mar 22, 2013

Member

Awesome, thanks for weighing in Joey!

Just a clarification, with the process option, I'd hope we could catch the current process before it fails and avoid spawning any new processes. See http://nodejs.org/api/process.html#process_event_exit I haven't tested this much with a watch task so not sure if it's a good option either. It's worth exploring because it's a solution that will work with any task and could be done now with the current version of Grunt.

Member

shama commented Mar 22, 2013

Awesome, thanks for weighing in Joey!

Just a clarification, with the process option, I'd hope we could catch the current process before it fails and avoid spawning any new processes. See http://nodejs.org/api/process.html#process_event_exit I haven't tested this much with a watch task so not sure if it's a good option either. It's worth exploring because it's a solution that will work with any task and could be done now with the current version of Grunt.

@joeybaker

This comment has been minimized.

Show comment
Hide comment
@joeybaker

joeybaker Mar 22, 2013

Contributor

hmm… interesting. I've not played with process.on('exit') either, is it possible to just never have the callback execute? Basically, we'd break process.exit()?

I see where you're going with that, but it seems like a bit of clever hack instead of an actual solution? Using Grunt to run a watch process is a pretty basic use-case, I'd really vote for Grunt itself having a solution to the problem.

Contributor

joeybaker commented Mar 22, 2013

hmm… interesting. I've not played with process.on('exit') either, is it possible to just never have the callback execute? Basically, we'd break process.exit()?

I see where you're going with that, but it seems like a bit of clever hack instead of an actual solution? Using Grunt to run a watch process is a pretty basic use-case, I'd really vote for Grunt itself having a solution to the problem.

@jlgrall

This comment has been minimized.

Show comment
Hide comment
@jlgrall

jlgrall Mar 23, 2013

You could first: Ask for specialized handling of failures in grunt.fail ?
And then until it is implemented, use a workaround like duck punching grunt.fail.

jlgrall commented Mar 23, 2013

You could first: Ask for specialized handling of failures in grunt.fail ?
And then until it is implemented, use a workaround like duck punching grunt.fail.

@joeybaker

This comment has been minimized.

Show comment
Hide comment
@joeybaker

joeybaker Mar 24, 2013

Contributor

@jlgrall that's probably best. FWIW, I originally had started the request to Grunt core here: gruntjs/grunt#706

Contributor

joeybaker commented Mar 24, 2013

@jlgrall that's probably best. FWIW, I originally had started the request to Grunt core here: gruntjs/grunt#706

@shama

This comment has been minimized.

Show comment
Hide comment
@shama

shama Apr 3, 2013

Member

I'm leaning towards doing this: process.exit = function() {};

That way the only way the watch will die is by hitting ctrl+c or if the process crashes. I can't think of a use case where the watch process would need to exit. It also solves a lot of task queuing problems (as the watch can solely rely on events rather than worrying about making sure to re-queue itself).

There used to be specialized handling in grunt.fail on v0.3 so I don't think it's likely to come back nor do I want to wait until v0.5 for it. So if we're going to duck punch something, why not the process itself? Or am I missing some dire consequences doing that?

Member

shama commented Apr 3, 2013

I'm leaning towards doing this: process.exit = function() {};

That way the only way the watch will die is by hitting ctrl+c or if the process crashes. I can't think of a use case where the watch process would need to exit. It also solves a lot of task queuing problems (as the watch can solely rely on events rather than worrying about making sure to re-queue itself).

There used to be specialized handling in grunt.fail on v0.3 so I don't think it's likely to come back nor do I want to wait until v0.5 for it. So if we're going to duck punch something, why not the process itself? Or am I missing some dire consequences doing that?

@joeybaker

This comment has been minimized.

Show comment
Hide comment
@joeybaker

joeybaker Apr 3, 2013

Contributor

Elegant. 👍

Contributor

joeybaker commented Apr 3, 2013

Elegant. 👍

@jlgrall

This comment has been minimized.

Show comment
Hide comment
@jlgrall

jlgrall Apr 4, 2013

It's so simple that it is worth a try.

jlgrall commented Apr 4, 2013

It's so simple that it is worth a try.

@KGZM

This comment has been minimized.

Show comment
Hide comment
@KGZM

KGZM Apr 12, 2013

Instead of overriding anything on process I opted for something similar to this:

grunt.registerTask('eatwarnings', function() {
  grunt.warn = grunt.fail.warn = function(warning) {
    grunt.log.error(warning)
  }
});

KGZM commented Apr 12, 2013

Instead of overriding anything on process I opted for something similar to this:

grunt.registerTask('eatwarnings', function() {
  grunt.warn = grunt.fail.warn = function(warning) {
    grunt.log.error(warning)
  }
});
@shama

This comment has been minimized.

Show comment
Hide comment
@shama

shama Apr 19, 2013

Member

Okay turns out I'm wrong and process.exit = function() {} isn't a good idea as it messes with test runners a bunch. I'll just duck punch warn and fatal sort of like @KGZM has done with an option to turn it off. At least it's something for now.

Member

shama commented Apr 19, 2013

Okay turns out I'm wrong and process.exit = function() {} isn't a good idea as it messes with test runners a bunch. I'll just duck punch warn and fatal sort of like @KGZM has done with an option to turn it off. At least it's something for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment