Added workaround for Node.js <= 6.2.1 bugs #4490
Conversation
It seems that Node.js <= 6.2.1 on Linux can improperly handle stdout stream of the spawned child process. This PR works around the issue, hiding the bug.
Testsuite on Node v4 was spitting out EventEmitter warnings about excessive amounts of listeners, this commit tries to reduce their amount.
|
This PR is starting to be ridiculous. Feel free to take it over and make it great again... |
| } | ||
|
|
||
| processClosed = true; |
There was a problem hiding this comment.
I guess this was a legitimate bug in Yarn code?
There was a problem hiding this comment.
I think the code was OK. When I was analyzing processClosed usage, it looked unnecessarily complex, but correct.
|
@Nopik I've pushed some changes. The code looks good to me but I'd be more comfortable if someone else also looks at this code. |
|
@Nopik seems like Node 6.2.1 is quite old. Maybe it is better to just update the website to urge people to upgrade? |
|
Yes, generally I would also encourage people to upgrade to latest 6.x (at least). Still, not supporting <6.2.2 (or being unreliable), isn't something I would recommend. Especially, when relatively easy workaround is there. |
|
@Nopik my argument is, this looks limited to Node >6 and <=6.2.1 range which is quite limited and Node 6 has an LTS that should be safe to upgrade. Do you think this is this too much to ask from users? |
|
Well, my own situation is that I'm stuck on 6.1.0 on CI with no easy upgrade path (due to stupid CircleCI limitations). So even if I have Node.js v8.x locally, I would fall into the 'wont upgrade' bucket. So, I might be wrong person to ask. Or, actually, the best one ;) Yarn has pretty wide audience now, and amount of people using the affected versions is probably at least few percent. I would try to get the support in place, but I will understand if Yarn core team would have different policy. |
I was wondering if we could reach out to CircleCI team to ask for an upgrade. The Node 6 builds we are running doesn't seem to be exhibiting this behavior.
I agree with your reasoning. That said adopting workarounds for older versions, where better alternatives are out there would both send the wrong message and in time, add up to the codebase to be cleaned up after an unknown time where we stop supporting. Don't get me wrong, I'm still undecided about merging or closing this PR. I'm just rying to gather more data about alternatives. |
**Summary** Fixes #4501, refs #4490, refs #4284. Yarn now warns when it detects it is running in a Node version that is not fully supported and warns the user about this. This is different than the hard Node 4+ check in the entry file since in that case, Yarn wuoldn't run at all due to syntax incompatibilities. This warning is to signal that users may encounter unexpected errors but are allowed to use Yarn if they wish. It also adds a new flag to suppress this warning: `--no-node-version-check`. **Test plan** Since we cannot add unsupported Node versions to our CI and spoof the Node version internally, this has to be tested manually, which I did.
I replied to your tweet @BYK but I'll respond here as well. Can Node.JS v8 solve this? We have two releases of that on CircleCI 1.0 available right now. The better, long-term solution is to switch to CircleCI 2.0 where you can't run into these types of issues. If you want, I can help with that after this PR is handled. |
|
I'm on CCI 2.0 (machine executor) and hitting exactly this type of issues. |
|
One the |
**Summary** Fixes #4501, refs #4490, refs #4284. Yarn now warns when it detects it is running in a Node version that is not fully supported and warns the user about this. This is different than the hard Node 4+ check in the entry file since in that case, Yarn wouldn't run at all due to syntax incompatibilities. This warning is to signal that users may encounter unexpected errors but are allowed to use Yarn if they wish. It also adds a new flag to suppress this warning: `--no-node-version-check`. **Test plan** Since we cannot add unsupported Node versions to our CI and spoof the Node version internally, this has to be tested manually, which I did.
|
Hey! Thanks a lot @Nopik for your investigation and fix, but I don't think we'll merge it - we have to set some limits to the versions we officially support, and while we have no issue using a function rather than another if it helps portability, I think we should avoid adding complex workarounds that only affect versions we don't support (especially when there are more recent LTS releases) - it makes the code more complex for use cases that would be better solved in other ways. To help a bit with this, @BYK pushed a commit that makes Yarn warn you when you're using it with an unsupported Node version. We believe it's a reasonable behavior for now, so I'll go ahead and close this PR. Thanks again for your help! 😃 |
**Summary** Fixes yarnpkg#4501, refs yarnpkg#4490, refs yarnpkg#4284. Yarn now warns when it detects it is running in a Node version that is not fully supported and warns the user about this. This is different than the hard Node 4+ check in the entry file since in that case, Yarn wouldn't run at all due to syntax incompatibilities. This warning is to signal that users may encounter unexpected errors but are allowed to use Yarn if they wish. It also adds a new flag to suppress this warning: `--no-node-version-check`. **Test plan** Since we cannot add unsupported Node versions to our CI and spoof the Node version internally, this has to be tested manually, which I did.
Summary
Fixes #4282. It seems that Node.js <= 6.2.1 on Linux can improperly handle stdout stream of the spawned child process. This PR works around the issue, hiding the bug.
Test plan
Git-related stuff should not fail or hang on Node <= 6.2.1 on Linux.