Large preprocessor outputs hit the maxBuffer limit #87

Merged
merged 5 commits into from Jul 13, 2013

Conversation

Projects
None yet
2 participants
@freejosh
Contributor

freejosh commented Jul 5, 2013

When using one main file that imports all my CSS with a preprocessor I was hitting the maxBuffer limit. I've changed exec to spawn so that the output gets built from the stream, without a limit.

Also fixed a typo in the regex that checks for Sass partials.

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Jul 5, 2013

Collaborator

Thanks for the request. Can you provide a regression test for child_process.spawn and resubmit?

Collaborator

marrs commented Jul 5, 2013

Thanks for the request. Can you provide a regression test for child_process.spawn and resubmit?

@freejosh

This comment has been minimized.

Show comment
Hide comment
@freejosh

freejosh Jul 6, 2013

Contributor

I'm not too sure how to do that, do I make a file like test/urlRelative.js? If that's what you mean, would you say it's fine to skip passing in an actual preprocessor command, and just run cat to read the contents of a large CSS file?

Contributor

freejosh commented Jul 6, 2013

I'm not too sure how to do that, do I make a file like test/urlRelative.js? If that's what you mean, would you say it's fine to skip passing in an actual preprocessor command, and just run cat to read the contents of a large CSS file?

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Jul 8, 2013

Collaborator

Yes, exactly. The urlRelative test is written using Mocha and Chai. The others are written in NodeUnit. You can use whichever you find more comfortable.

If reading the contents of a large file directly still reproduces the error that you fixed then I would say it's preferable to skip the pre-processor command. Ideally we want the fewest/simplest conditions to reproduce the error.

Collaborator

marrs commented Jul 8, 2013

Yes, exactly. The urlRelative test is written using Mocha and Chai. The others are written in NodeUnit. You can use whichever you find more comfortable.

If reading the contents of a large file directly still reproduces the error that you fixed then I would say it's preferable to skip the pre-processor command. Ideally we want the fewest/simplest conditions to reproduce the error.

@freejosh

This comment has been minimized.

Show comment
Hide comment
@freejosh

freejosh Jul 12, 2013

Contributor

Let me know if adding the tests here was fine, or if they should be in a separate pull request, to test with unfixed code first.

Contributor

freejosh commented Jul 12, 2013

Let me know if adding the tests here was fine, or if they should be in a separate pull request, to test with unfixed code first.

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Jul 12, 2013

let's leave this test out. It's useless without the stub and is passing in CI by coincidence only. The remaining tests are good.

let's leave this test out. It's useless without the stub and is passing in CI by coincidence only. The remaining tests are good.

@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Jul 12, 2013

Collaborator

Here is fine. I trust that you watched the test fail and then made it pass. :)

Although, now that I understand the nature of the bug, I'm wondering if it's worth including this test. I'm not sure I want to add a 200M stub to the repo just to reproduce, but without it the test is at best useless.

Let's merge the branch without the test. I think a better idea for the future is to hook a linter into the test suite to check the code for child_process.exec and warn the user against using it.

Collaborator

marrs commented Jul 12, 2013

Here is fine. I trust that you watched the test fail and then made it pass. :)

Although, now that I understand the nature of the bug, I'm wondering if it's worth including this test. I'm not sure I want to add a 200M stub to the repo just to reproduce, but without it the test is at best useless.

Let's merge the branch without the test. I think a better idea for the future is to hook a linter into the test suite to check the code for child_process.exec and warn the user against using it.

@freejosh

This comment has been minimized.

Show comment
Hide comment
@freejosh

freejosh Jul 12, 2013

Contributor

Sounds good. Yea the warning should be something like:

exec, by default, only returns 200k of data. Use spawn if you're expecting larger return data.

Contributor

freejosh commented Jul 12, 2013

Sounds good. Yea the warning should be something like:

exec, by default, only returns 200k of data. Use spawn if you're expecting larger return data.

marrs added a commit that referenced this pull request Jul 13, 2013

Merge pull request #87 from freejosh/master
Large preprocessor outputs hit the maxBuffer limit

@marrs marrs merged commit a2235d5 into jacobrask:master Jul 13, 2013

1 check passed

default The Travis CI build passed
Details
@marrs

This comment has been minimized.

Show comment
Hide comment
@marrs

marrs Jul 13, 2013

Collaborator

Thanks again for the patch. It'll be in the next maintenance release

Collaborator

marrs commented Jul 13, 2013

Thanks again for the patch. It'll be in the next maintenance release

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