Repository navigation
in RemoveDisposeScope, we can't assume that DisposeScopeStack.Peek() == disposeScope #1303
Description
using TorchSharp;
var d1 = torch.NewDisposeScope();
var d2 = torch.NewDisposeScope();
d1.Dispose();
var tensor = torch.zeros([]);
d2.Dispose();
tensor.print();
// It could print.
// So this tensor leaks.Also:
using TorchSharp;
var d = torch.NewDisposeScope();
d.Dispose();
d.Dispose();
// InvalidOperationException
// But exceptions are always not expected when disposing something.
Activity
NiklasGustafsson commentedon May 1, 2024
The canonical way to create a DisposeScope should be
using var d1 = ...I don't think we should allow either one of those patterns you show, but if Dispose() shouldn't throw an exception, then I don't know how to enforce it properly. I do understand the PR now, but I'd rather we looked for a way to disallow improper use.
yueyinqiu commentedon May 1, 2024
I suppose this won't cause any problem, and we could just allow it.
Another possible design is to also dispose the nested scope when disposing the outer one. But this cannot avoid the possible misuse in the first example.
Or we could just add a
ifstatement to checkDisposeScopeStack.Peek() == disposeScope. If not, just do nothing.Hmm, this also seems a bad idea. My suggestion is to just allow it.
NiklasGustafsson commentedon May 1, 2024
Let's allow it for now.
yueyinqiu commentedon May 1, 2024
I have made it ready for review.