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

WHATWG URL Memory Leak #19019

Closed
aikar opened this issue Feb 26, 2018 · 8 comments
Closed

WHATWG URL Memory Leak #19019

aikar opened this issue Feb 26, 2018 · 8 comments
Labels
memory Issues and PRs related to the memory management or memory footprint. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@aikar
Copy link
Contributor

aikar commented Feb 26, 2018

I've been debugging this issue: sidorares/node-mysql2#726
I'm confident the leak is in native land, as my heap grew over the weekend from 21.8MB to 22.0MB, comparing Heap Snapshots with the Chrome Inspector, shows no signs of a leak.

However the RSS of my process grew 240MB (in the course of the weekend)

I've narrowed the issue down to being related to the URL parsing of the Database DSN, as when I did a core dump of my leaking process,
I found my hostnames at 1.3 million instances of the string, and thousands to hundreds of thousands of various chunks of my DSN.
mysql2 lib does url parsing like so:
https://github.com/sidorares/node-mysql2/blob/master/lib/connection_config.js#L167

Seems pretty standard to me.

edit: mysql2 notes not relevant, see below comments.

Related Issue: #17448

@aikar
Copy link
Contributor Author

aikar commented Feb 26, 2018

It appears the related issue I linked deals with the WHATWG impl, where as node-mysql2 is using the original implementation.

I'll see if I can swap it to the WHATWG and hopefully it will resolve it for me, but i'm a bit confused about what in the old implementation is leaking the strings since it's JS oriented.

@ChALkeR ChALkeR added url Issues and PRs related to the legacy built-in url module. memory Issues and PRs related to the memory management or memory footprint. labels Feb 26, 2018
@aikar
Copy link
Contributor Author

aikar commented Feb 27, 2018

It appears I am using the WHATWG version in my applications code const url = new URL(dsn); and then passing the result to node-mysql2, so the issue is at the WHATWG implementation, and not the JS one, as mysql2 isn't even doing URL parsing given that I am passing it an object form instead of string DSN.

@devsnek devsnek added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 27, 2018
@ChALkeR ChALkeR removed the url Issues and PRs related to the legacy built-in url module. label Feb 27, 2018
@ChALkeR
Copy link
Member

ChALkeR commented Feb 27, 2018

@aikar Could you provide a complete minimal testcase that reproduces this issue without mysql2?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 27, 2018

The following testcase seems to reproduce the issue on v8.9.4 but not affect v9.5.0.
@aikar Could you confirm that v9.5.0 is not affected in your setup?

const { URL } = require('url');

let i = 0;

function tick() {
  const url = 'mysql://user:pass@host/db?debug=true&charset=BIG5_CHINESE_CI&timezone=-0700&rand=' + Math.random();
  new URL(url);
  if (++i % 1e6 === 0) console.log(i);
  setImmediate(tick);
}

tick();

@TimothyGu TimothyGu added security Issues and PRs related to security. and removed security Issues and PRs related to security. labels Feb 27, 2018
@TimothyGu
Copy link
Member

Sounds like the same issue as #17448.

The fix for #17448, #17470, is included in:

  • v6.13.0+ (the first v6.x version that introduced the WHATWG URL parser)
  • No v8.x versions, but should be in the next v8.x version (cc @MylesBorins)
  • v9.4.0+

@richardlau
Copy link
Member

Sounds like the same issue as #17448.

The fix for #17448, #17470, is included in:

  • v6.13.0+ (the first v6.x version that introduced the WHATWG URL parser)
  • No v8.x versions, but should be in the next v8.x version

cc @gibfahn @nodejs/lts

@MylesBorins
Copy link
Contributor

It is already staged in c2028fa and will be out in the next 8.x release --> #18336

Does it make sense to close this or wait for the release to be cut?

@ChALkeR ChALkeR added the v8.x label Feb 27, 2018
@aikar aikar changed the title URL.parse memory leak WHATWG URL Memory Leak Mar 7, 2018
@aikar
Copy link
Contributor Author

aikar commented Mar 7, 2018

Closing as fix has landed in 8.10

@aikar aikar closed this as completed Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
memory Issues and PRs related to the memory management or memory footprint. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests

6 participants