Skip to content
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

Closed
MylesBorins opened this issue Mar 28, 2017 · 21 comments
Closed

Regressions in v4.8.1 and v6.10.1 #193

MylesBorins opened this issue Mar 28, 2017 · 21 comments

Comments

@MylesBorins
Copy link
Contributor

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)

  • 😀 Get an R.C. out this week and release next week (not giving the leak two weeks to sit)
    • only include the fixes for regressions + leak
  • 🎉 Get an R.C. out this week and release in two weeks
    • only include the fixes for regressions + leak
  • ❤️ Get an R.C. out next week and release in three weeks
    • include all commits that would normally be in a release

I'm leaning towards the first option to fix this asap.

@giannosdev
Copy link

😀

@MylesBorins
Copy link
Contributor Author

the release of v7.8.0 went out just now. Will prep rcs for v4.8.2 and v6.10.2 tomorrow

@aarogrammer
Copy link

@clocked0ne
Copy link

clocked0ne commented Mar 29, 2017

Hope this does fix it!
nodejs/node#12019

😀

@gibfahn
Copy link
Member

gibfahn commented Mar 29, 2017

@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!)

@jasnell
Copy link
Member

jasnell commented Mar 29, 2017

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.

@ZibbyKeaton
Copy link

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.

@gibfahn
Copy link
Member

gibfahn commented Mar 29, 2017

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.

Why not? Seems odd to suggest someone use v6.10.1 given that it has a known issue, when we could just suggest v6.10.0:

image

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.

@fhemberger
Copy link

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.)

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 30, 2017

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.

@depoulo
Copy link

depoulo commented Mar 30, 2017

there are other regressions in v6.10.0

wow - good to know...

@mhdawson
Copy link
Member

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.

@bnoordhuis
Copy link
Member

bnoordhuis commented Mar 31, 2017

I mean it's not really true to say it's "recommended for most users" right?

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?

@dmabamboo
Copy link

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.

@gibfahn
Copy link
Member

gibfahn commented Mar 31, 2017

@bnoordhuis

What is more important? What is used more?

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).


I'd say it is.

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.

And while inconvenient, it's just a memory leak, not a segfault or a security vulnerability.

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).

@andris9
Copy link

andris9 commented Mar 31, 2017

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 tls.connect({socket:existingSocket}).

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.

@bnoordhuis
Copy link
Member

So your position is that the http abort fix is more important?

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.

Don't memory leaks cause Node to crash when it runs out of memory?

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.

@asanchezdelc
Copy link

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.

@richardlau
Copy link
Member

@asanchezdelc 6.10.2 and 4.8.2 were released today and contain fixes for the memory leak.

@asanchezdelc
Copy link

@richardlau beautiful thanks!

@MylesBorins
Copy link
Contributor Author

Closing as we have fixed the leak in latest v4 and v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests