-
Notifications
You must be signed in to change notification settings - Fork 578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regressions in v4.8.1 and v6.10.1 #193
Comments
😀 |
the release of v7.8.0 went out just now. Will prep rcs for v4.8.2 and v6.10.2 tomorrow |
❤ |
Hope this does fix it! 😀 |
@nodejs/lts given the issues with 6.10.1 and 4.8.1, presumably we don't want to continue to have 4.8.1 and 6.10.1 front and center on https://nodejs.org/ and https://nodejs.org/en/download/. Is there an easy way to roll back? cc/ @nodejs/website A blog post explaining the issue and the versions with problems would probably be useful too. (If this is already being done somewhere else and I just missed it then ignore me!) |
I don't believe we've ever rolled back the website links based on a regression and not sure we should get in the habit of doing so. We could have a banner displayed above the links like we do for security updates tho. Emphasis should definitely be on getting fixed builds out either way. |
I like @gibfahn's idea of providing a blog on this postmortem. There tends to be a lot of confusion in releases with Node.js and folks might need some guidance on how to upgrade. I'm happy to draft an outline of what might be helpful to provide folks here, but wouldn't have the knowledge to really fill it in with the technical details. There's been a few questions on this on social and once the blog is updated we can share, so folks can get their questions answered. Also, on social someone mentioned that it's taking a lot of time detecting changed files, is this due to the issue? I want to make sure we are communicating why issues may be happening and to follow this thread to follow updates. |
Why not? Seems odd to suggest someone use I mean it's not really true to say it's "recommended for most users" right? At the moment unless people see a message on Twitter, they have no way of knowing that this is not a build they should be using. |
Node versions are pulled from the version.json on website build. To roll back we have to modify the script to use a fixed version number instead. (I'm on the road ATM, so I can't do it myself, sorry.) |
If we were to roll it back we should likely move to v6.9.x... as there are other regressions in v6.10.0 edit: you could make a commit that hard codes the version for the time being and then revert it next week. |
wow - good to know... |
I definitely agree that updating the website so we don't people at releases they should not use is the right way to go. We might want to plan for an easier way to do it in the future. |
I'd say it is. You're staring yourself blind on that memory leak but that only affects the subset of users that make outbound TLS connections. And while inconvenient, it's just a memory leak, not a segfault or a security vulnerability. Meanwhile v6.10.1 contains commits such as nodejs/node@28102ed that fixes a hard abort in the http module. What is more important? What is used more? |
You shouldn't promote a known faulty version of your product unless you clearly state the issues (which are quite damaging). A memory leak will always cause a service to be degraded or interrupted give it load. Most services nowadays consume other services via HTTPS so I large number of Node users should be affected. |
I don't know which one is more severe, but judging from the reaction to nodejs/node#11015, it doesn't seem like there was any particular hurry to backport that to LTS, and the issue was only reported by one person as far as I can tell. The memory leak was reported within a few days by multiple people as causing serious issues, and we've reacted by making an exception to our normal LTS backporting policy (we're backporting after a couple of days), even shipping another v4.x release despite the fact that it's about to go into maintenance and we weren't planning to do any more normal releases. Overall we're treating this as a severe regression, which leads me to believe that overall we'd be more comfortable suggesting people use an earlier version of v6 (I don't know what was wrong with v6.10.0, but given that we waited a month before shipping v6.10.1 I suspect it's not as big an issue).
So your position is that the http abort fix is more important? I don't know, but from reading the linked issues I thought the consensus was that this issue is more important than others we've had recently.
Don't memory leaks cause Node to crash when it runs out of memory? Is that less of an issue than segfaults (again, serious question). |
For us the memory leak was a real problem. We send out about 500 000 emails a day using our own open source MTA (ZoneMTA). ~90% of the connections against MX servers use STARTTLS, so the connections are upgraded with So far I had to restart the processes twice a day to keep RAM usage under control. Today I upgraded from 6.10.1 to 7.8.0 only to get rid of the memory leak. Everything is running smoothly since that upgrade, no leaks anymore. |
I picked it at random. It's just one of several fixes that went into v6.10.1. As long as you're not affected by the memory leak, it's the version I'd recommend.
Yes, but only when you hit the leak repeatedly. Node.js has memory limits so that is something that can happen during normal operation too, if the application needs more memory than the limits allow. Deployment scripts are usually written with that in mind. |
We have noticed the same memory leak on our systems while doing 6.10.1 and 4.8.1 testing. This should definitely be addressed. |
@asanchezdelc 6.10.2 and 4.8.2 were released today and contain fixes for the memory leak. |
@richardlau beautiful thanks! |
Closing as we have fixed the leak in latest v4 and v6 |
So it would appear that there is a memory leak in v4.8.1 and v6.10.1
nodejs/node#12033
A fix has been made with a very high probability of solving all memory leak issues
nodejs/node#12089
There was also a handful of regressions in V8 on v6.10.x
nodejs/node#11977
nodejs/node#12017
nodejs/node#12018
They have already all been fixed by a backport from V8
nodejs/node#12037
The plan currently is to land the memory leak fix today and hopefully get a v7.x cut today as well.
Assuming that goes out we should work on getting a v4.x and v6.x release out ASAP
Should we (vote with emoji)
I'm leaning towards the first option to fix this asap.
The text was updated successfully, but these errors were encountered: