Resolve FilterFixture NRE

[whoisj]

Resolve racy NRE via re-architect of filter logic.

Moves all filter logic into the Filter class. Made Filter into IDisposable.

Seperates registration logic into the FilterRegistration class. Made FilterRegistration into IDisposable.

Moves core registration logic into FilterRegistration while leaving legacy methods in GlobalSettings. Methods in GlobalSettings now forward to methods in FilterRegistration.

Retains handles on all filter and registration values in static FilterRegistration class to prevent native code calling disposed managed objects resulting in NRE. Manages register, deregister, and dispose logic coherently.

Added a new test to verify improved features and Survivability.

Fixes a few other areas where NRE happened after GC collection vs. native utilization race occurred.

Fixes #1091

[whoisj]

/cc @nulltoken , @shiftkey , @ammeep

Minor API redesign here. Need you to please take a look. Thanks!

[nulltoken]

Tons of thanks for this! ✨

I'll take a deeper look tomorrow. But below a quick first feedback:

• I'd rather keep the registration as a responsibility of GlobalSettings rather than making FilterRegistration expose public static methods (I'd rather see everything which lifecycle exceeds a Repository time span in one place)
• How about not making Filter and FilterRegistration implement IDisposable and rather adding a finalizer to FilterRegistration (see Add a finalizer SmartSubtransportRegistration #1017)
• Maybe could we turn GetRegisteredFilters() into a static IEnumerable<> property of GlobalSettings?

[nulltoken]

@jamill You've successfully survived some garbage collection wars before, Could you please glance at this?

Kudos and ❤️ to @jeffhostetler and his fine-tuned psychic debugger: This was indeed GC related!

[whoisj]

I'd rather keep the registration as a responsibility of GlobalSettings rather than making FilterRegistration expose public static methods (I'd rather see everything which lifecycle exceeds a Repository time span in one place)

While I can agree with this, it means adding a lot of methods to a structure called "settings". I'm not sure if that is logical in and of itself. Honestly, I only left the methods in GlobalSettings as a backwards compatibility feature.

If we really want all the global code to move to GlobalSettings the change is trivial and I'll be happy to make it.

How about not making Filter and FilterRegistration implement IDisposable and rather adding a finalizer to FilterRegistration (see #1017)

Finalizers are non-deterministic. That means racy - while I do not have a problem having a finalizer that calls this.Dispose, I'm rather concerned about leaving memory allocated longer than it needs to be - especially when it's native memory.

As a contributor to a rather large-ish app, I much prefer to have the ability to dispose of my disposable objects when I determine it is the right time, and not be at the mercy of the GC.

Things to consider when dealing with finalizers / finalize operations have the following limitations:

• The exact time when the finalizer executes is undefined. To ensure deterministic release of resources for instances of your class, implement a Close method or provide a IDisposable.Dispose implementation.
• The finalizers of two objects are not guaranteed to run in any specific order, even if one object refers to the other. That is, if Object A has a reference to Object B and both have finalizers, Object B might have already been finalized when the finalizer of Object A starts.
• The thread on which the finalizer runs is unspecified.
• You should override Finalize for a class that uses unmanaged resources such as file handles or database connections that must be released when the managed object that uses them is discarded during garbage collection.
• Finalize might not be called during shutdown of an application or during DLL_PROCESS_DETACH state.

Maybe could we turn GetRegisteredFilters() into a static IEnumerable<> property of GlobalSettings?

I originally considered an IEnumerable<T> but there are two issues with it:

1. It's racy. Libgit2Sharp isn't multi-threaded buy many of the code bases that utilize it are. So either we're still doing a copy to array in the background, or we're being thread-unsafe.
2. IEnumerable<T> doesn't expose a cheap .Count or .Length property - potentially causing an enumeration of the entire collection to return an int.

Again a trivial change. I recommend performing the copy to prevent any long term locking or racy-ness in the global variable. If we want to expose it as an IEnumerable<FilterRegistration> instead of a FilterRegistration[] that's fine too.

[whoisj]

OK, so I've pushed an update with most of @nulltoken was asking for so that we can look at the version side-by-side.

• All global operations done in GlobalSettings
• Finalizers on Filter and FilterRegistration
• GetRegisteredFilters returns a generic instead of an instance type
• Remove IDisposable.Dispose methods in favor of finalizers
• GetRegisteredFilters returns an IEnumerable<RegisteredFilter>
• GetRegisteredFilters as a property instead of a method

The first three are done because they're trivial and mostly have no impact on the library itself. The second three are pending additional feedback.

I prefer enabling the developer to control the disposal of memory, thus have retained the IDisposable interface. I used IList<RegisteredFilter> instead of IEnumerable<RegisteredFilters> because of the cost reduction of not needing LINQ and guaranteeing the array won't get iterated over just for a count of items. Finally, I left GetRegisteredFilters as a method to signal its cost of execution and as a hint for developers to not for (int i = 0; i < GetRegisterdFilters.Count; i++) because that would be horrible.

[nulltoken]

While I can agree with this, it means adding a lot of methods to a structure called "settings". I'm not sure if that is logical in and of itself. Honestly, I only left the methods in GlobalSettings as a backwards compatibility feature.

Indeed, maybe xxxSettings isn't the correct suffix. Now would be a GOOD time to change it. 😉

Finalizers are non-deterministic.
As a contributor to a rather large-ish app, I much prefer to have the ability to dispose of my disposable objects when I determine it is the right time, and not be at the mercy of the GC.

I wasn't suggesting to not free it as soon as possible. Just that I'd rather not multiply the number of public IDisposable types. The finalizer would just be an additional security layer.

Considering the usage of filters, beside very simple code samples, I don't see them being wrapped into a using() statement. So having to Unregister() rather than Dispose() the instance wouldn't really be a big downer, would it?

I originally considered an IEnumerable but there are two issues with it:

It's racy.

You're right, of course, we should copy and return this copy as an IEnumerable.

IEnumerable doesn't expose a cheap .Count

AFAIR Linq .Count() introspects the underlying type and for some of them (IList, ICollection, ...) delegate the job to the the proper/efficient property.

[whoisj]

Indeed, maybe xxxSettings isn't the correct suffix. Now would be a GOOD time to change it. 😉

Indeed, but not in this pull-request. If we're assuming a rename could be in order, then I agree - let's move all the logic to GlobalSettings for now.

I wasn't suggesting to not free it as soon as possible. Just that I'd rather not multiply the number of public IDisposable types. The finalizer would just be an additional security layer.

😞 not sure what to do here. Having discrete control of memory is rather important from my point-of-view. If the dev prefers to allow the finalizer to do the work, great - but enabling the devs to clean up memory as necessary is powerful and immensely useful, and we lose nothing by implementing it.

If we keep the Filter as IDisposable then the we need a way to instantly unregister the FilterRegistration. I suppose it does not need to be an IDisposable.Dispose() implementation, but anything that isn't one is in many ways just reinventing the wheel because we can.

Am I making sense? I'm never quite sure. 😕

AFAIR Linq .Count() introspects the underlying type and for some of them (IList, ICollection, ...) delegate the job to the the proper/efficient property.

Unfortunately, I don't believe this to be true. Look at corefx/Enumberable.cs @ line 1489. It calls MoveNext() on every element in the array, while counting.

        public static int Count<TSource>(this IEnumerable<TSource> source)
        {
            if (source == null) throw Error.ArgumentNull("source");
            ICollection<TSource> collectionoft = source as ICollection<TSource>;
            if (collectionoft != null) return collectionoft.Count;
            ICollection collection = source as ICollection;
            if (collection != null) return collection.Count;
            int count = 0;
            using (IEnumerator<TSource> e = source.GetEnumerator())
            {
                checked
                {
                    while (e.MoveNext()) count++;
                }
            }
            return count;
        }

I highly recommend that we stick with the IList<T> implementation.

_EDIT_

Nevermind, reading code is hard. Due to the magic of if (collectionoft != null) return collectionoft.Count;, @nulltoken is correct and IEnumerable<T> is smarter than I originally thought. 😀

[nulltoken]

Unfortunately, I don't believe this to be true.

@whoisj What about this code part?

ICollection<TSource> collectionoft = source as ICollection<TSource>;
if (collectionoft != null) return collectionoft.Count;
ICollection collection = source as ICollection;
if (collection != null) return collection.Count;

[jamill]

As we (the lg2# framework)now keep a collection of the registered filters, do they need to be disposable? They should not need a finalizer (except to handle logic errors in the library?), as lg2# is now managing the lifetime of these objects, and can make sure the correct clean up is done when they are removed.

[whoisj]

@whoisj What about this code part?

Yeah - see my addendum. 😖

As we now keep a collection of the registered filters, do they need to be disposable? They should not need a finalizer (except to handle logic errors in the library?), as the library is now managing the lifetime of these objects, and can make sure the correct clean up is done when they are removed.

Keeping the objects alive as long as we need them is independent of enabling devs to manage their memory. They have a strong cross section, but see them as independently valuable aspects.

[nulltoken]

As an array implement ICollection, I believe the code can be simplified to

registeredFilters = GlobalSettings.GetRegisteredFilters();
Assert.Equal(0, registeredFilters.Count());

[whoisj]

Do we want ICollection<T> or IEnumerable<T>? 😕

[nulltoken]

@whoisj Linq Count() implementation tests is the underlying type implements ICollection. When that's the case, it will directly invoke the Count property from it.

Thus, we can make GetRegisteredFilters return and IEnumerable<> and let the tests leverage the Linq Count() extension method.

This should also allow us to get rid of the cast as IList<FilterRegistration> in the tests.

So the following code

var registeredFilters = GlobalSettings.GetRegisteredFilters() as IList<FilterRegistration>;
Assert.Equal(1, registeredFilters.Count);

can be rewritten as

var registeredFilters = GlobalSettings.GetRegisteredFilters();
Assert.Equal(1, registeredFilters.Count());

Sorry if I was unclear earlier.

[whoisj]

Ahh yeah, just add the using System.Linq; in the header and "magic!"

[nulltoken]

Global nitpick: Could you please switch the following code constructs

Assert.Equal(assertedVariable, 0);

into

Assert.Equal(0, assertedVariable);

XUnit defines the expected value as the first parameter. Following this pattern makes the assertion failure far more easy to understand 😉

[whoisj]

XUnit defines the expected value as the first parameter. Following this pattern makes the assertion failure far more easy to understand 😉

Yes, and another framework I was using them has this way - I agree, making the error message sensible is a good thing. Why can't these framework writers just agree on ordering?! 😜

[whoisj]

@nulltoken if we're good with the final code, I can squash for a merge. Let me know.

[nulltoken]

@whoisj I'm still uneasy with IDisposable filters. Could we let GlobalSettings handle their lifecycle?

[whoisj]

I'm still uneasy with IDisposable filters.

I'd like to understand your unease. I'm likely just not understanding something obvious. I know that I can be overly zealous about use of IDisposable, maybe it is clouding my thoughts on the topic?

I'm moved all the native allocations into the Filter class such that they can be managed coherently and via standard, library-provided mechanisms. This was rather intentional, under the belief that C# devs have been trained by fire to respect and understand IDisposable.

Per MSDN, IDisposible is defined as "provid[ing] a mechanism for releasing unmanaged resources."

Could we let GlobalSettings handle their lifecycle?

I think GlobalSettings should participate in managing the lifecycle of the Filter objects. In fact, it is in the design now. If a Filter becomes unregistered, it gets disposed - freeing up native allocations and invalidating the managed object.

Can you explain your logic behind the reluctance to using IDisposable?

[ethomson]

IDisposeable or no, it seems like there are several ways to deregister a filter now, either by a call to GlobalSettings or by disposing the filter itself, which does the same thing. It seems like this would benefit more by having a single mechanism to deregister, rather than a couple.

Note that this would also be the only GlobalSetting that deregisters itself via IDIsposeableness - the others have matching register and deregister calls. This seems like it would be nice parity to maintain.

[nulltoken]

👍 to what @ethomson wrote.

I've got also some other points to expose, but the answer will likely take me more than five minutes to write.

Let me sleep over this and I'll write down my thoughts tomorrow to make my reasoning clearer.

[whoisj]

@nulltoken, @ethomson OK that makes more sense, but I we'll still need a way to reclaim filters that leave scope - perhaps an internal implementation of Dispose is sufficient?

Remember, the reason we were leaking was because the tests were releasing their handles on filters without deregistering them. While the tests are fixed up now, I foresee this as a common problem for app devs.

I suppose with the ledger this is less meaningful. I'll wait for you response in the AM and follow whatever course of action get this issue resolved 😉

[jamill]

With the ledger, now LG2# is responsible for cleaning up the native resources when the filter is deregistered. If a filter go out of scope and the native resources are not cleaned up, then this would indicate (quite a serious) issue in the framework. Consumers of LibGit2Sharp (App devs?) should not have a problem with the native resources not being cleaned up. They might still have an issue with not deregistering the filter.

I think something that implements something similar to the dispose pattern internally would be nice (so at least we test / assert our correct behavior).

[nulltoken]

I think something that implements something similar to the dispose pattern internally would be nice (so at least we test / assert our correct behavior).

👍

[whoisj]

OK so I've changed the code so that unregistering a filter releases its native memory and removed the reliance on IDisposable. It invalidates the following fact, are we OK with that?

        [Fact]
        public void CanRegisterAndUnregisterTheSameFilter()
        {
            var filter = new EmptyFilter(FilterName, attributes);

            var registration = GlobalSettings.RegisterFilter(filter);
            GlobalSettings.DeregisterFilter(registration);

            var secondRegistration = GlobalSettings.RegisterFilter(filter);
            GlobalSettings.DeregisterFilter(secondRegistration);
        }

/CC @ammeep because she's the author of the fact.

[ethomson]

That's not the mechanism that I would have expected. Why doesn't RegisterFilter allocate the native stuff and DeregisterFilter free it, instead of the constructor?

[whoisj]

That's not the mechanism that I would have expected. Why doesn't RegisterFilter allocate the native stuff and DeregisterFilter free it, instead of the constructor?

Why would git_filter_register() allocate git_filter? Given that logic and git_write_stream management is entirely in the Filter class, putting the native allocation into another class breaks coherency.

The RegsiterFilter and UnregisterFilter methods only use the associated git_filter_register and git_filter_unregister methods respectively. Allocation happens when the dev allocates the object, RAII (or as close as .NET gets).

[ethomson]

I'm not making a claim as to what class(es) things should live in.

[nulltoken]

@whoisj I don't really care about C# type/C function names coherency. From an API perspective, this is an implementation detail and shouldn't influence the design.

[nulltoken]

This was rather intentional, under the belief that C# devs have been trained by fire to respect and understand IDisposable.

Unfortunately, I don't believe that most c# devs properly understand/honor IDisposable contract. This is why huge effort as been made in order to expose only one public IDisposable type. The Repository acts as an aggregate root and takes care of the heavy lifting for the developer.

I share @ethomson's views. We should only expose one true way to do things. Easier to troubleshoot for us, less confusing for the consumer.

Things livings in GlobalSettings hold contracts that exceed the lifespan of a single repository. Those are dangerous things to play with. It would make sense to see them all managed under one umbrella type, where we add documentation and huge warnings (see #1119 (comment)) rather than having each of those types being able to Dispose() themselves. The developer should "feel" a bit wary when leveraging those behaviors. Let's make the ⚠️ signal easier for him to spot.

[whoisj]

@nulltoken The IDisposable stuff has already been removed, so I think we're good there.

@whoisj I don't really care about C# type/C function names coherency. From an API perspective, this is an implementation detail and shouldn't influence the design.

Just so that I'm clear here, you're saying you want native allocation to happen at filter registration and not creation - is that correct?

[nulltoken]

Just so that I'm clear here, you're saying you want native allocation to happen at filter registration and not creation - is that correct?

Indeed. It should be safe to create a filter for a user. From his/her perpective, it shouldn't be anything more than some POCO.

[nulltoken]

@whoisj BTW, Could you please take a look at the failing builds?

[whoisj]

@whoisj BTW, Could you please take a look at the failing builds?

Per my comment above, it'll fail on that test until I revert my change. I'll get it fixed up asap (likely early next week).

[whoisj]

@nulltoken all fixed up per your request. Let me know if you need anything else. If not, I'll squash and resubmit.

[nulltoken]

@whoisj 👍 Looks pretty neat to me. Thanks for all the hard work ❤️

@jamill @ethomson Any comments?

[nulltoken]

@whoisj Can you please squash?

[nulltoken]

Maybe add a

Assert.True(registration.isValid);

[whoisj]

@whoisj Can you please squash?

@nulltoken, done.

[nulltoken]

💎