Implement IDisposable in OdbBackend

[nulltoken]

Seeing how it implements Dispose, it doesn't make sense not to have it implement IDisposable

[ethomson]

It does make sense; the lifecycle for an OdbBackend is such that libgit2 itself is the one who disposes the backend, not the caller.

[synhershko]

Yes, unless you're writing your own and want to properly test it (my case). Anyway, the framework can do nice things with disposables, so why not tell it it is one?

[ethomson]

Well, maybe I'm not understanding what you want to do. But if you were to make that Dispose method public, and make the class IDisposable, then as soon as you disposed the OdbBackend, you'd get an access violation when libgit2 called in to your OdbBackend the next time.

I guess I just don't understand what problem you're trying to solve.

[synhershko]

Implement my own OdbBackend and unit-test it, and currently I cannot do:

using (var be = new MyOdbBackend())
{
// test
}

Even though it does implement Dispose()

In what scenarios do you see ppl going to dispose the backend and expecting libgit2 not to throw???

[ethomson]

Yes, but it's not clear to me why you want to do that. We also implement our own OdbBackends, we also unit test them, and making them IDisposable or not does not impact that.

Right now: there are no scenarios where people can dispose the backend. Only libgit2 can dispose the backend. That's why the Dispose method is not public.

[synhershko]

Because testing a backend through libgit2 is not unit testing :) there are a few behaviors I'm testing directly, at least as smoke tests. It would be easier to have a disposable object implement IDisposable but I can see your point for not buying that (although I disagree with the approach).

Feel free to close this if the answer is still no.

[dahlbyk]

Similar to #528 (comment), perhaps we should reserve Dispose for truly IDisposable use cases and use something else (Free?) for cleanup managed by libgit2?

It would be easier to have a disposable object implement IDisposable but I can see your point for not buying that (although I disagree with the approach).

Nothing prevents you from making MyOdbBackend implement IDisposable to simplify your unit testing.

[synhershko]

Free like in C or in Beer ? :)

[synhershko]

And good point @dahlbyk

[ethomson]

Aha aha - I see. Yeah, I think that what @dahlbyk is suggesting is the best solution, that:

1. We should not call this method Dispose because it is very confusing and might get in the way with you implementing IDisposable in your OdbBackend
2. Then you can implement IDisposable (independent of the method that libgit2 calls when it's done with your backend)

Does this seem reasonable?

[synhershko]

@ethomson yep, but try finding a better name than Free :)

[ethomson]

Okay, that's fair. I'm partial to Free but if you have a better idea,
I'm all ears.

Thanks for your patience in explaining this problem; apologies for being
dense.

On Mon, May 19, 2014 at 4:09 PM, Itamar Syn-Hershko <
[email protected]> wrote:

@ethomson https://github.com/ethomson yep, but try finding a better
name than Free :)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/713#issuecomment-43569534
.

[nulltoken]

Ok. I turned this issue into a PR with a proposal.

I changed the way OdbBackend deals with the release of its own resources while making it easier for the consumer to create its own IDisposable backend.

Previous implementation was allowing the consumer to override Dispose(), but there was a risk of leak if the consumer forgot to call base.Dispose().

The change allows the following:

• No funky Dispose() method exposed
• OdbBackend will always release its own resources
• If the backend implements IDisposable, the Dispose() method will be called

[nulltoken]

Implement my own OdbBackend and unit-test it, and currently I cannot do:

using (var be = new MyOdbBackend())
{
// test
}

Even though it does implement Dispose()

@synhershko I wouldn't recommend this kind of unit testing. AFAIK, Read() and ReadPrefix() will delegate to libgit2 the memory allocation (through Allocate()) and only libgit2 is in charge of releasing the memory when it's done with it.

Leveraging the backend by itself, outside of the libgit2sharp/libgit2 ecosystem would short-circuit the memory life cycle and lead to memory leaks.

Well, at least this is my understanding of the code... and I may be completely wrong.

/cc @ethomson

[nulltoken]

[txdv]

I propose to rename the variable to pointerBackendNative

[phkelley]

This looks good. The only thing I would say is that sometimes with Dispose methods there are bugs where the environment invokes the Dispose method twice. It might be interesting to modify the test case to check for a stronger guarantee -- that the dispose count on the MockOdbBackend is exactly 1 after the Repository is disposed, rather than just checking that it has been invoked at least once, which is what is happening now. This is only a suggestion and should not hold up this PR, because what you have here is fine as is as well.

[nulltoken]

@phkelley You're right. Fixed! Thanks for your 👀 !