search

passport-next / passport

Simple, unobtrusive authentication for Node.js.
MIT License
36 stars 5 forks source link

Possible memory leak #15

Closed rwky closed 5 years ago

rwky commented 5 years ago

From https://github.com/jaredhanson/passport/issues/711

I've verified same thing happens with passport-next/passport

adamhathcock commented 5 years ago

Thanks. I’m going to look into this myself but hopefully they fix it upstream first.

I’m relatively new to node so it’s weird to me jest would find a leak just from importing

rwky commented 5 years ago

It's unlikely upstream will look into it, the reporter might but the maintainers most likely won't. Due to require being what triggers it that limits the code to inspect to the code at initilization, assuming there is a leak at all since the memory flag in jest is marked as experimental.

adamhathcock commented 5 years ago

One of the requirements is "use of a global variable" which doesn't necessarily mean a leak and I believe passport itself is a global singleton.

rwky commented 5 years ago

Comment this out https://github.com/passport-next/passport/blob/master/lib/index.js#L16 if the error goes away it's probably a false positive

adamhathcock commented 5 years ago

Hah. I guess they have a lot more to do to "detect memory leaks" in node.

rwky commented 5 years ago

Well it is 'experimental' ;)

rwky commented 5 years ago

I've had a play and it's caused by the monkey patching here https://github.com/passport-next/passport/blob/master/lib/framework/connect.js#L34

It doesn't matter what the functions that extend the prototype are e.g. http.IncomingMessage.prototype.logIn = function() {}; will also trigger it.

Whether this is actually causing a memory leak I don't know but I suspect fiddling with the prototype might cause problems and we've an issue out to remove monkey patching so fixing #9 should also fix this.