Output on a pipe stops at 64K #286

Closed
dolmen opened this Issue May 11, 2016 · 10 comments

Projects

None yet

4 participants

@dolmen
dolmen commented May 11, 2016

On Linux, when the output of the js-yaml CLI is a pipe to another command, the output is truncated to the first 65536 bytes.

Here is how to reproduce (with any big enough YAML file):

$ wget http://static.snoyman.com/entries.yaml     # A 190K YAML file
$ js-yaml entries.yaml > entries.json ; wc -c entries.json
245736 entries.json
$ js-yaml entries.yaml | wc -c
65536

Tested with node v6.1.0 (arch: amd64) and js-yaml 3.6.0 with bash 4.3.11(1) on Ubuntu 14.04.

@puzrin puzrin added the bug label May 11, 2016
@puzrin
Member
puzrin commented May 11, 2016

Hm... commenting out this line https://github.com/nodeca/js-yaml/blob/3.6.0/bin/js-yaml.js#L133 fixed the problem. Honestly, nobody cared too much about cli.

@ixty, blame shows this shit magic with process.exit() first happened in your commit 4 years ago :) . Do you have any ideas why?

@dervus
Collaborator
dervus commented May 11, 2016

@puzrin there's a typo in your reference @ixti

@ixti
Contributor
ixti commented May 11, 2016

I don't understand how proper process exit status may be responsible.
But generally that particular line is not really important indeed.
Just love to be verbose ;)) sometimes...

@puzrin
Member
puzrin commented May 11, 2016

It forces process exit while it still have data in pipe buffer.

@ixti
Contributor
ixti commented May 11, 2016

So synchronous call behaves asynchronous... Terrific!

Anyway - just remove that line then.

@puzrin puzrin closed this in 2b5e2b3 May 11, 2016
@dolmen
dolmen commented May 12, 2016

A flush on STDOUT before exit also would probably have fixed the issue. This is probably done by the stdlib before ending the process.

@puzrin
Member
puzrin commented May 12, 2016

@dolmen do you know how to do that in node?

@dervus
Collaborator
dervus commented May 12, 2016

Isn't 0 the default exit code anyway?

@puzrin
Member
puzrin commented May 12, 2016

yes

@dolmen
dolmen commented May 13, 2016

@puzrin No I don't.

Thanks for the fix. I confirm the issue is closed in js-yaml@3.6.1.

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