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

Warn of potential memory leaks #2357

Merged
merged 4 commits into from Apr 22, 2021

Conversation

@DullReferenceException
Copy link
Contributor

@DullReferenceException DullReferenceException commented Apr 21, 2021

Purpose (TL;DR) - mandatory

  • Warn on the console if a potential memory leak is detected
  • Allow adjusting of the leak detection threshold

Fixes #2356

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. Call sinon.stub() 10,001 times in a loop
  4. Verify that a console warning is output

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.
- Warn on the console if a potential memory leak is detected
- Allow adjusting of the leak detection threshold
@mantoni
Copy link
Member

@mantoni mantoni commented Apr 22, 2021

Great! Thank you 👍

Can I ask you to use printWarning from our commons library?
https://github.com/sinonjs/commons/blob/e909e9746c32ba6c3c67e64b9ca5b62644cf343e/lib/deprecated.js#L48

This is safer and tests across various platforms to behave nicely.

I'm also not sure if the default threshold of 1000 isn't a bit low, since APIs like sinon.createStubInstance create lots of instances at once. What do you think @mroderick?

@DullReferenceException
Copy link
Contributor Author

@DullReferenceException DullReferenceException commented Apr 22, 2021

Updated, bumping default threshold to 10000 and using deprecated.printWarning instead.

@fatso83
Copy link
Contributor

@fatso83 fatso83 commented Apr 22, 2021

Great stuff and something that should have been there long ago. Not totally sold on the track() naming, though. I think it was easier to reason about what push(collection, spy) did than track(spy), so I would have preferred if the tracking bits were internal details of "pushing into internal collection", as the latter is the main point. Maybe a simple rename is all that's needed (addToCollection(spy)?). These is all quite minor details, though, and we could merge it as is.

@DullReferenceException
Copy link
Contributor Author

@DullReferenceException DullReferenceException commented Apr 22, 2021

Good point on naming. Renamed to addToCollection as suggested.

@fatso83 fatso83 merged commit 77df7f3 into sinonjs:master Apr 22, 2021
8 checks passed
@fatso83
Copy link
Contributor

@fatso83 fatso83 commented Apr 22, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants