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

Make all tasks asynchronous to reduce call stack #1026

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@piuccio
Contributor

piuccio commented Jan 14, 2014

I'm having a weird error when running grunt with lot of tasks (~500).

The issue happens while running grunt-uglify but the root cause is the high number of multi tasks.

[RangeError: Maximum call stack size exceeded]
>> Uglifying source "rtl/dist/widget.history.unpack.js" failed.
Warning: Uglification failed.
Maximum call stack size exceeded. 
 Use --force to continue.
Aborted due to warnings.

With this pull request all tasks are now asynchronous and next task will run in a 'new' stack after the next tick.

This is very similar to gruntjs/grunt-contrib-imagemin#132

@eddiemonge

This comment has been minimized.

Show comment
Hide comment
@eddiemonge

eddiemonge Jan 14, 2014

Contributor

just out of curiosity, what are you doing that requires 500 tasks?

Contributor

eddiemonge commented Jan 14, 2014

just out of curiosity, what are you doing that requires 500 tasks?

@vladikoff

This comment has been minimized.

Show comment
Hide comment
@vladikoff

vladikoff Jan 14, 2014

Member

@piuccio Thanks for the PR, marked as part of 0.4.3, we will look at this.

Member

vladikoff commented Jan 14, 2014

@piuccio Thanks for the PR, marked as part of 0.4.3, we will look at this.

@piuccio

This comment has been minimized.

Show comment
Hide comment
@piuccio

piuccio Jan 15, 2014

Contributor

@eddiemonge it's not the best project organization ever, but I have few thousands files in a very large project.
Files are analyzed by a task that generates the grunt configuration for concatenation and minification from a dependency management system.

This task generates a grunt uglify task for each destination file, and recently we passed the threshold of maximum stack size (~500).

Contributor

piuccio commented Jan 15, 2014

@eddiemonge it's not the best project organization ever, but I have few thousands files in a very large project.
Files are analyzed by a task that generates the grunt configuration for concatenation and minification from a dependency management system.

This task generates a grunt uglify task for each destination file, and recently we passed the threshold of maximum stack size (~500).

@cowboy cowboy closed this in cc9941a Jan 22, 2014

@silvenon

This comment has been minimized.

Show comment
Hide comment
@silvenon

silvenon Mar 11, 2014

Won't this break stuff like tests?

Here's an example test from grunt-usemin which is now failing:

  it('should work on CSS files', function () {
    grunt.file.mkdir('images');
    grunt.file.mkdir('images/misc');
    grunt.file.write('images/test.23012.png', 'foo');
    grunt.file.write('images/misc/test.2a436.png', 'foo');
    grunt.log.muted = true;
    grunt.config.init();
    grunt.config('usemin', {css: 'style.css'});
    grunt.file.copy(path.join(__dirname, 'fixtures/style.css'), 'style.css');
    grunt.task.run('usemin');
    grunt.task.start();

    var changed = grunt.file.read('style.css');

    // Check replace has performed its duty
    assert.ok(changed.match(/url\(\"images\/test\.23012\.png\"/));
    assert.ok(changed.match(/url\(\"images\/misc\/test\.2a436\.png\"/));
    assert.ok(changed.match(/url\(\"\/\/images\/test\.23012\.png\"/));
    assert.ok(changed.match(/url\(\"\/images\/test\.23012\.png\"/));
  });

Sorry, I know there's a lack of context here, but I'm pretty sure these assertions fail because grunt.task.run('usemin') now runs asynchronously and files didn't change yet. Is there an obvious way to run these assertions when the usemin task is done?

silvenon commented Mar 11, 2014

Won't this break stuff like tests?

Here's an example test from grunt-usemin which is now failing:

  it('should work on CSS files', function () {
    grunt.file.mkdir('images');
    grunt.file.mkdir('images/misc');
    grunt.file.write('images/test.23012.png', 'foo');
    grunt.file.write('images/misc/test.2a436.png', 'foo');
    grunt.log.muted = true;
    grunt.config.init();
    grunt.config('usemin', {css: 'style.css'});
    grunt.file.copy(path.join(__dirname, 'fixtures/style.css'), 'style.css');
    grunt.task.run('usemin');
    grunt.task.start();

    var changed = grunt.file.read('style.css');

    // Check replace has performed its duty
    assert.ok(changed.match(/url\(\"images\/test\.23012\.png\"/));
    assert.ok(changed.match(/url\(\"images\/misc\/test\.2a436\.png\"/));
    assert.ok(changed.match(/url\(\"\/\/images\/test\.23012\.png\"/));
    assert.ok(changed.match(/url\(\"\/images\/test\.23012\.png\"/));
  });

Sorry, I know there's a lack of context here, but I'm pretty sure these assertions fail because grunt.task.run('usemin') now runs asynchronously and files didn't change yet. Is there an obvious way to run these assertions when the usemin task is done?

@silvenon

This comment has been minimized.

Show comment
Hide comment
@silvenon

silvenon Mar 12, 2014

I remove all uncertainty from my previous comment. This is a breaking change introduced in a minor version update.

silvenon commented Mar 12, 2014

I remove all uncertainty from my previous comment. This is a breaking change introduced in a minor version update.

@cowboy

This comment has been minimized.

Show comment
Hide comment
@cowboy

cowboy Mar 12, 2014

Member

I'm not sure how to handle this. For starters, the grunt.task.start() method isn't really part of the public API and is only meant to be used internally. I definitely didn't intend people to use Grunt like this, but instead to test their plugins like we do with the contrib series.

@tkellen I'm wondering if there is a way we can revert this change where we made it and make it somewhere else, so that people can avoid this problem. Maybe we can add an option to grunt.task.start() that only Grunt uses internally like {async: true} to make this happen. Do you have any time to look at this today? I'm still teaching a class here.

Member

cowboy commented Mar 12, 2014

I'm not sure how to handle this. For starters, the grunt.task.start() method isn't really part of the public API and is only meant to be used internally. I definitely didn't intend people to use Grunt like this, but instead to test their plugins like we do with the contrib series.

@tkellen I'm wondering if there is a way we can revert this change where we made it and make it somewhere else, so that people can avoid this problem. Maybe we can add an option to grunt.task.start() that only Grunt uses internally like {async: true} to make this happen. Do you have any time to look at this today? I'm still teaching a class here.

@tkellen

This comment has been minimized.

Show comment
Hide comment
@tkellen

tkellen Mar 12, 2014

Member

I'll take a look here shortly.

Member

tkellen commented Mar 12, 2014

I'll take a look here shortly.

@silvenon

This comment has been minimized.

Show comment
Hide comment
@silvenon

silvenon Mar 12, 2014

I have no idea what grunt.task.start() does, I was searching everywhere.

silvenon commented Mar 12, 2014

I have no idea what grunt.task.start() does, I was searching everywhere.

@tkellen

This comment has been minimized.

Show comment
Hide comment
@tkellen

tkellen Mar 12, 2014

Member

@silvenon I will push a patch release to resolve this issue shortly.

Member

tkellen commented Mar 12, 2014

@silvenon I will push a patch release to resolve this issue shortly.

@tkellen

This comment has been minimized.

Show comment
Hide comment
@tkellen

tkellen Mar 12, 2014

Member

@silvenon This has been resolved in 0.4.4. Thanks for letting us know about this, and sorry for the regression!

Member

tkellen commented Mar 12, 2014

@silvenon This has been resolved in 0.4.4. Thanks for letting us know about this, and sorry for the regression!

@silvenon

This comment has been minimized.

Show comment
Hide comment
@silvenon

silvenon commented Mar 12, 2014

Cool :D

@vladikoff vladikoff modified the milestones: 0.4.4, 0.4.3 May 9, 2014

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