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

Emit a warning if sandbox restore has not been called or have a non-sandbox API #2356

Closed
DullReferenceException opened this issue Apr 21, 2021 · 7 comments · Fixed by #2357
Closed

Comments

@DullReferenceException
Copy link

Is your feature request related to a problem? Please describe.
Our team has gone many many years using this wonderful software unaware that sinon retained references to fakes in a global sandbox. It's documented well, but either we didn't find our way to that specific page before or we just didn't RTFM properly. It wasn't until our OOM issues got really frustrating with a large test suite that we finally figured out that we weren't restoring this global sandbox.

Describe the solution you'd like
There are two things that could have helped us here:

  • Outputting a warning if the global sandbox accumulates some threshold of object fakes, notifying the user that you may have a memory leak due to not releasing the global sandbox
  • Have a non-sandboxed API available. We have never needed/used the sandbox because we don't usually stub/spy live objects, and when we do we retain a reference to the stub and restore it ourselves. It is not intuitively obvious that if you have const someStub = sinon.stub().returns('foo'); that it's retained elsewhere in case you wanted to do a bulk operation on all your fakes.
@mantoni
Copy link
Member

mantoni commented Apr 21, 2021

I like the idea of a (configurable?) threshold for registered fakes in the default sandbox.

Would you like to send a PR for this to help avoid this frustration for others?

@DullReferenceException
Copy link
Author

Sure, will try to make the time for that.

@mroderick
Copy link
Member

Wouldn't it be better if manually restoring stubs didn't cause memory leaks in the first place?

Did you look into a solution that removes the reference from the sandbox that is tracking the stub?

@DullReferenceException
Copy link
Author

DullReferenceException commented Apr 22, 2021

The problem isn't that individual restores was causing a memory leak; the problem is that we were never restoring the fakes or the sandbox because it's not intuitively obvious that you need to do so.

It makes sense that if you're stubbing live objects (sinon.stub(someObject, 'prop')) that you'd need to restore these stubs either individually or within the context of a sandbox because you need to bring those objects back to their original state. But why does sinon need to keep track of standalone sinon.stub() or sinon.spy() instances? Regular garbage collecting should be sufficient. Needing to call .restore() or sandbox.restore() to dereference standalone fakes is counter-intuitive. Hence my proposed pull request clues in a developer that they need to restore the sandbox.

@mroderick
Copy link
Member

What I meant was that we could have found a solution under the hood, that would remove the stub from the collection when you call it's own .restore() method.

Nothing new for the users to worry about and no messages in the console either.

I think #2357 got merged too soon, before other options had been explored.

Ping @fatso83

@DullReferenceException
Copy link
Author

DullReferenceException commented Apr 23, 2021

Well, in our case we weren't calling each fake's own .restore() method. Our assumption was that .restore() was for restoring live objects that had been stubbed, not for dereferencing even standalone fakes. Our memory leak was from not knowing about the behavior of all fakes being collected until released with sandbox.restore() or possibly individual restores. I still think a warning that you're most likely leaking is super useful. It's only slightly annoying that we have to call sinon.restore() but very annoying that it was hard to figure out that was the problem.

@fatso83
Copy link
Contributor

fatso83 commented Apr 23, 2021

What I meant was that we could have found a solution under the hood, that would remove the stub from the collection when you call it's own .restore() method.

That assumes you restore something. That is not given. As here.

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 a pull request may close this issue.

4 participants