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:
const someStub = sinon.stub().returns('foo'); that it's retained elsewhere in case you wanted to do a bulk operation on all your fakes.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?
Sure, will try to make the time for that.
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?
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.
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
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.
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.
Most helpful comment
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 withsandbox.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 callsinon.restore()but very annoying that it was hard to figure out that was the problem.