fix: use write instead of end to show results

[lachrist]

Using process.stdout.end may disturb the normal closing of the node process. It is safer to use process.stdout.write. @apotterri is there a good reason to keep using process.stdout.end? As far as I know, it is the only place in your code that may cause the early exit of node.

[apotterri]

What problems does using end cause when closing node?

If we must switch to using write instead, the code needs to handle backpressure from the stream. If it does not, the CLI will fail when it tries to use the status subcommand (because the output from status is bigger than some internal buffer).

[lachrist]

First, writable.end is internally calling writable.write so I don't see how it can better handle large input -- cf src. Second, the return value of writable.write is advisory only. The node process will run out of memory before loosing data -- cf doc. Since the entirety of the data is already present in the node process it is safe to write it in one chunk.

I'm not sure that calling process.stdout.end is the cause of the issue. But it is the only thing that seems fishy in your code. Moreover, unless there is good reason for it, I would consider closing the stdout a bad practice and would leave this responsibility to node. So this change would be worthwhile even if it does not fix the issue.

[lachrist]

From a node team member:

That behavior is intentional and fixes more issues than it introduces. Most programs fail very badly if stdio could be closed and in fact they did. That's why we made stdio permanent in v0.6.0.

-- nodejs/node#7606

[apotterri]

So, have you seen an actual failure as a result of the change to using end?

[lachrist]

No I can't reproduce the faulty behavior on my setup. I just have the intuition that it is where the problem lies. So this is what I propose: we make this simple change and check whether we keep seeing the issue in the telemetry. If it stops great. If it continues, we are still happy to have removed process.stdout.end and we try something else. Do we have anything better to do?

[apotterri]

No I can't reproduce the faulty behavior on my setup. I just have the intuition that it is where the problem lies.
Which problem are you referring to?

I don't think we should make this change as written. I switched from write to end because it fixed a real failure that showed up in the telemetry (and that I was able to reproduce). Despite what the doc says, write was only writing part of the data, then returning false.

I'm not against changing this code. I just don't think the changes in this PR are sufficient.

[lachrist]

I think the error you saw earlier was due to me calling process.exit in the bin file. Now I'm using process.exitCode = status which is cleaner because it let node exit on its own which gives it the opportunity to flush internal buffers. I'm confident the issue you observed previously would not re-appear.

I don't see anything else to do, this change is my best guest.

[apotterri]

Ok. Let me see if I can reproduce the problem using these changes.

[apotterri]

I confirmed that this does, indeed, fix the problem, at least in my local environment.

[lachrist]

Okay great, I will mark the issue as solved. Let's keep an eye on telemetry and reopen the issue if we see the same problem passing by.

[appland-release]

🎉 This PR is included in version 10.0.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀