Fix output buffer is imcomplete working in child_process #1834

ysugimoto · 2016-12-13T03:55:21Z

When I send a large buffer, over 50KB's CSS string, working in child_process.spawn, node-sass outputs only partial strings.

Certainly, it is caused by nodejs's console.log max size.
nodejs/node#9546

So I modified to send output by continuous partial strings, its asynchronous.

This PR will fix this issue: #1823.

xzyfer · 2016-12-13T05:08:47Z

bin/node-sass

  });

  emitter.on('done', function() {
-    if (!options.watch && !options.directory) {
+    if (!options.watch && !options.directory && !asyncWaiting) {


isn't asyncWaiting always true now?

asyncWaiting is initialized on false when called getEmitter().
https://github.com/sass/node-sass/pull/1834/files#diff-c19ab2481e2703fcf0d2808f4c762f63R147

I read project source code, emit log event only stdout case ( is it right? ).
So that, asyncWaiting set to be true only emitted log event.

Please advise if there are other concerns that you need to worry about. thanks!

I don't think this is right. I believe we always want to exit when emitter.log is called. This is what happens when you call the node-sass cli. If I understand correctly all we need to do is

emitter.on('log', stdout.write);

Am I missing something?

xzyfer · 2016-12-13T05:14:08Z

https://www.npmjs.com/package/stdout-stream seems like a better alternative than writing out own stdout stream lib.

ysugimoto · 2016-12-13T06:25:31Z

I replaced out stdout lib to suggested package, it also works fine!

ysugimoto · 2016-12-13T06:43:22Z

It works fine, but Travis CI is down for some testings...

xzyfer · 2016-12-13T09:03:23Z

I'm sorry the issue here is not clear.

Either the problem is related to console.logs max buffer size, in which we can fix by

emitter.on('log', stdout.write)

It's because process.stdout is no longer blocking, in which case we can use https://github.com/isaacs/block-stream. Either way i don't think this is the correct solution.

ysugimoto · 2016-12-13T09:16:28Z

Yes, I think so.

However, stdout.write is no longer blocking, then we stop process.exit until stdout stream output completely. It's called by emitter.emit('done') immediately, So I add the flag of asyncWaiting to guard process exit.

xzyfer · 2016-12-13T09:32:38Z

OK I did some digging. You're correct that the issue is due how process.exit works.

It is important to note that calling process.exit() will force the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

I see ways to solve this:

remove the process.exit (I don't think we need it)
use exit instead

ysugimoto · 2016-12-13T10:08:28Z

Thank you for your advice. I replaced to process.exit to exit package.

And I removed exiting in emitter.on('done', ...) callback because of process exited even though using stdout-stream package, I wondering...
After removed this point, It works fine: CLI, spawned process, and watch mode.

xzyfer · 2016-12-13T10:26:57Z

bin/node-sass

@@ -161,13 +163,12 @@ function getEmitter() {
  });

  emitter.on('log', function(data) {


emitter.on('log', stdout.write)

xzyfer · 2016-12-13T10:27:38Z

bin/node-sass

@@ -161,13 +163,12 @@ function getEmitter() {
  });

  emitter.on('log', function(data) {
-    console.log(data);
+    // In nodejs <= 7.1.0, console.log cannot write output larger than 8275 bytes data.
+    stdout.write(data);
  });

  emitter.on('done', function() {


Please delete the emitter.on('done' block all together.

xzyfer · 2016-12-13T10:28:28Z

bin/node-sass

@@ -150,7 +152,7 @@ function getEmitter() {
    }
    console.error(err);
    if (!options.watch) {
-      process.exit(1);
+      exit(1);


process.exit is fine here because console.error is synchronous.

ysugimoto · 2016-12-13T10:36:49Z

https://github.com/sass/node-sass/pull/1834/files#diff-c19ab2481e2703fcf0d2808f4c762f63R165

Use bind() because of avoid error on stdout-stream.

xzyfer · 2016-12-13T10:39:25Z

package.json

@@ -56,6 +56,7 @@
    "async-foreach": "^0.1.3",
    "chalk": "^1.1.1",
    "cross-spawn": "^3.0.0",
+    "exit": "^0.1.2",


Please remove.

xzyfer · 2016-12-13T10:49:41Z

lib/render.js

@@ -47,21 +47,14 @@ module.exports = function(options, emitter) {
  var stdin = options.stdin;

  var success = function(result) {
-    var todo = 1;


The done still needs to be fired. It's used in other places. Please undo these changes and just remove

emitter.on('done', function() { // noop });

xzyfer · 2016-12-13T11:09:11Z

Thank you for your work and patience. I'll merge this when CI is happy.

Fix premature exit before the output stream is fully flushed

ysugimoto added 3 commits December 13, 2016 12:39

Fix problem on stdout workin in child_process

a887833

Fix comment, unused variable

9eab386

Fix eslint

2ba1755

xzyfer reviewed Dec 13, 2016

View reviewed changes

Replace our stdout lib to stdout-stream package

e729c9f

Replace process.exit to exit pakcage

bbe7c75

xzyfer reviewed Dec 13, 2016

View reviewed changes

ysugimoto added 2 commits December 13, 2016 19:33

Remove done events

51d6d6d

console.error is synchronous

9f93835

xzyfer reviewed Dec 13, 2016

View reviewed changes

Remove exit package

a00ab7a

xzyfer reviewed Dec 13, 2016

View reviewed changes

Revert done

2f778ba

xzyfer merged commit 3c17b03 into sass:master Dec 13, 2016

xzyfer added the Module - CLI label Dec 13, 2016

xzyfer mentioned this pull request Dec 13, 2016

Only partial data sent to process.stdout when output reaches a certain size #1823

Closed

gmaliar pushed a commit to gmaliar/node-sass that referenced this pull request Dec 17, 2016

Fix output buffer is imcomplete working in child_process (sass#1834)

ec5aabc

Fix premature exit before the output stream is fully flushed

renovate-tests mentioned this pull request Jul 7, 2017

Update dependency node-sass to v4 renovate-tests/discover-package-files#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix output buffer is imcomplete working in child_process #1834

Fix output buffer is imcomplete working in child_process #1834

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016

ysugimoto Dec 13, 2016

xzyfer Dec 13, 2016

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016 •

edited

Loading

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016 •

edited

Loading

xzyfer Dec 13, 2016

xzyfer Dec 13, 2016 •

edited

Loading

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016

xzyfer Dec 13, 2016

xzyfer commented Dec 13, 2016

		@@ -161,13 +163,12 @@ function getEmitter() {
		});

		emitter.on('log', function(data) {

Fix output buffer is imcomplete working in child_process #1834

Fix output buffer is imcomplete working in child_process #1834

Conversation

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016

Choose a reason for hiding this comment

ysugimoto Dec 13, 2016

Choose a reason for hiding this comment

xzyfer Dec 13, 2016

Choose a reason for hiding this comment

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016 • edited Loading

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

xzyfer Dec 13, 2016

Choose a reason for hiding this comment

xzyfer Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

ysugimoto commented Dec 13, 2016

xzyfer Dec 13, 2016

Choose a reason for hiding this comment

xzyfer Dec 13, 2016

Choose a reason for hiding this comment

xzyfer commented Dec 13, 2016

ysugimoto commented Dec 13, 2016 •

edited

Loading

xzyfer Dec 13, 2016 •

edited

Loading

xzyfer Dec 13, 2016 •

edited

Loading