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

Replace Array.filter with Array.splice, to prevent the creation of a new... #184

Closed
wants to merge 3 commits into from

Conversation

lmarkus
Copy link

@lmarkus lmarkus commented Nov 24, 2014

... Array object. A new Array object might cause in-memory references to be lost, and potentially leak memory.

This should fix krakenjs/kraken-js#285

All tests passed using tap test/versioned/express*/*.tap.js except one because it coudn't connect to some Redis instance I don't have installed.

A more complete explanation of this can be found here:
https://github.com/lmarkus/newrelicbuster

…new Array object. A new Array object might cause in-memory references to be lost, and potentially leak memory
@wraithan
Copy link
Contributor

Thanks, we may want to add a test to make sure we don't replace the array in the future since kraken depends on that. Could also just add a kraken test that showed that this was a problem. We can write the tests, but wont happen this week probably. Or if you write them we should be able to merge it for release this week baring any other issues.

@lmarkus
Copy link
Author

lmarkus commented Nov 24, 2014

Unit test added.
Fails on the old code, passes on the patched code.
It's pretty straight forward.
Using express-enrouten (which is the actual Kraken module with the issue) try to load two routes.
The first one will always work, because the memory reference hasn't been removed yet.
The second route will not load on the current version, but will load with the patched code.

@wraithan
Copy link
Contributor

@lmarkus thanks! Do you want to be in the release notes, and if so, how would you like to be attributed. Lots of folks use github username, others prefer full name, we are happy to attribute to you however you'd like.

@lmarkus
Copy link
Author

lmarkus commented Nov 24, 2014

Lenny Markus works for me.
Thanks!

@hayes
Copy link
Contributor

hayes commented Nov 25, 2014

released as 1.14.0

@hayes hayes closed this Nov 25, 2014
bizob2828 added a commit to bizob2828/node-newrelic that referenced this pull request Apr 19, 2024
bizob2828 added a commit to bizob2828/node-newrelic that referenced this pull request Apr 23, 2024
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

Successfully merging this pull request may close these issues.

Newrelic doesn't work with kraken
3 participants