fix ENOTCONN error in Node v10.1.0

[cristianberroeta]

Node v10.1.0 throws an error when process.stdin.end() is called.
This PR replaces process.stdin.end() with process.stdin.destroy() to fix the issue.

For more information about the bug, visit:
workshopper/stream-adventure#213

[lupomontero]

Hi there, trying to use this workshopper in NodeSchool Lima and we all get the "shutdown ENOTCONN" error on Node v10.x. Any news on this PR?

[cristianberroeta]

Hi Lupo!

It seems like there is no maintainer for the adventure repo, as @martinheidegger told me in the workshoppers gitter, so this PR might take some time to be accepted. By the way, if anyone is interested in maintaining it, I suppose she/he could ask there.

For now, I think that some temporary solutions are:

1. installing nvm, and then running:

nvm install 10.0.0
nvm use 10.0.0

You should use 10.0.0 or any other version not in the 10.1.0 - 10.?.? problematic range. By the way, the root cause of this problem seems to be in the node repo, and apparently it will be fixed once the “libuv patch” is applied, as they said in this pull request.

2. creating a virtual machine (with virtual box, vagrant, or another) and installing a node version not in the 10.1.0 - 10.?.? problematic range there

3. manually editing lib/menu.js, just like this pull request does: https://github.com/workshopper/adventure/pull/15/files. It just replaces end() with destroy()

Best wishes @lupomontero , and thanks again for your presentation here in NodersJS (Santiago, Chile) last month!

[lupomontero]

BTW, I just checked the stream-adventure workshopper with node v11.0.0 and it no longer throws an error, yay 🚀

However, the workshopper doesn't exit automatically when it should (pressing the enter key makes the program exit tho). I've tested this PR with Node v11.0.0 and it seems to solve this problem too.

process.stdin is a Readable stream, and as far as I can tell, according to the docs, the Readable constructor/class doesn't implement an .end() method, only Writable streams implement the .end() method. In fact, while testing this on Node v11 I also discovered that the .end property in Readable streams holds the value Infinity (:confused: :cold_sweat:).

[cristianberroeta]

I also don't get an error when using v11.0.0, and have to press enter (or some key combination, like Ctrl+d or Ctrl+c) to exit the program. That's at least an improvement :)

This part of the nodejs docs is complex. I don't fully understand it at all. But It seems like process.stdin is a net.Socket (which is a Duplex stream) in our case, whereas in some other cases it is just a Readable stream, considering what the docs say here (I don't know how fd0 could refer to a file, though). I ran a sample code to confirm that process.stdin is a net.Socket, a stream.Duplex, a stream.Readable and a stream.Writable.

Anyway, I think that it would be preferable to apply this PR, so that we use the less troublesome destroy() instead of the end() method.

[lupomontero]

@cristianberroeta: totally agree that it would be preferable to apply this PR 👍

Today we are having another NodeSchool in Lima and some attendees are still running into this issue...

@julianduque, I can see you were the last person to commit to master, is there any chance you (or anyone with write access) could look at this PR? Many thanks in advance.

On the other hand, I'd like to volunteer to help maintain this repo if it helps ✋

cc/ @ccarruitero

[ccarruitero]

+1 to merge this PR.

Like @lupomontero mentioned, we were using this workshopper in recently nodeschool's events in Lima.

Aside, I would like to help maintaining this repo too.

cc/ @julianduque @martinheidegger

[cristianberroeta]

Thanks for merging @martinheidegger!

Will you also update the npm adventure package to version 2.11.2?

[martinheidegger]

https://github.com/workshopper/org/blob/master/DEPLOYMENT.md

[cristianberroeta]

Thanks @martinheidegger! I hadn't noticed that deployment process.

This repo only complies with 1 of the 4 pre-requisites mentioned there :(

For now, can I just create a request ticket for deployment, as this PR seems to be an improvement and highly unlikely to generate an error?

But afterwards it will probably be useful to comply with those 3 other deployment pre-requisites:

• using travis,
• adding tests,
• and providing a changelog

[martinheidegger]

For now, can I just create a request ticket for deployment, as this PR seems to be an improvement and highly unlikely to generate an error?

Basically yes - since you are maintaining this. (these are just strong suggestions) btw. this is also an organisation question 😉

[cristianberroeta]

Thanks @martinheidegger! I've just created the ticket.