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

Add private method ToDisposableLinkedList to replace DisposableGroup and Acquire #724

Open
Orace opened this issue Nov 22, 2019 · 3 comments

Comments

@Orace
Copy link
Contributor

Orace commented Nov 22, 2019

Transpose, Interleave and SortedMerge are three methods that have to deals with enumeration of a collection of IEnumerable.

In this three methods, implementation have to ensure that all enumerators from the input enumerables are disposed when:

  1. The enumeration of the input collection of IEnumerable fail.
  2. The result sequence enumerator is disposed.

1. is assured by Acquire in the three methods.
2. is assured by DisposableGroup in SortedMerge and by some try {} finaly {} blocks in Transpose and Interleave (and those make Acquire useless as discussed here #696 ).

Acquire return an array, DisposableGroup is based on an list.
For the three methods we can build cases where enumerators are heavily removed from the array/list (or, for Transpose, tested as null).
Since remove for array/list (or repeatedly skip null elements) is O(N) in time, that lead to O(N²) time complexity.
This can be easily fixed with the use of a LinkedList (and by removing null elements from it).

I propose a ToDisposableLinkedList() method that 1. have the functionality of Acquire (on error, dispose already acquired elements) and provide a DisposableLinkedList that 2. have the functionality of DisposableGroup<T> (being disposable and dispose its content when disposed).

It will be used like that:

using var enumerators = source.Select(e => e.GetEnumerator()).ToDisposableLinkedList();

The using here allow to remove the try {} finally {} from here:

finally
{
foreach (var e in enumerators)
e?.Dispose();
}

and here:
finally
{
Debug.Assert(iteratorList != null || iterators != null);
foreach (var iter in (iteratorList ?? (IList<IEnumerator<T>>) iterators))
iter.Dispose();
}

and improve the DisposableGroup/Acquire combo from here:

using var disposables = new DisposableGroup<TSource>(sequences.Select(e => e.GetEnumerator()).Acquire());

@Orace
Copy link
Contributor Author

Orace commented Nov 22, 2019

A first step is to use the Disposable/Acquire combo in Transpose and Interleave.

@atifaziz
Copy link
Member

This is just an idea but Acquire can be overloaded to return any type of collection, including a LinkedList<>:

public static TCollection Acquire<TSource, TCollection>(this IEnumerable<TSource> source,
                                                        TCollection disposables)
    where TSource : IDisposable
    where TCollection : ICollection<TSource>
{
    if (source == null) throw new ArgumentNullException(nameof(source));

    try
    {
        foreach (var disposable in source)
            disposables.Add(disposable);
        return disposables;
    }
    catch
    {
        foreach (var disposable in disposables)
            disposable.Dispose();
        throw;
    }
}

And if you have a DisposableCollection like this:

sealed class DisposableCollection<T> : ICollection<T>, IDisposable
    where T : IDisposable
{
    readonly LinkedList<T> _disposables = new LinkedList<T>();

    public void Dispose()
    {
        foreach (var disposable in this)
            disposable.Dispose();
        Clear();
    }

    public int Count => _disposables.Count;
    public bool IsReadOnly => false;
    public void Add(T item) => _disposables.AddLast(item);
    public void Clear() => _disposables.Clear();
    public bool Contains(T item) => _disposables.Contains(item);
    public void CopyTo(T[] array, int arrayIndex) => _disposables.CopyTo(array, arrayIndex);
    public IEnumerator<T> GetEnumerator() => _disposables.GetEnumerator();
    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    public bool Remove(T item) => _disposables.Remove(item);
}

then it can also be used with Acquire.

@Orace
Copy link
Contributor Author

Orace commented Dec 12, 2019

As explained here, Acquire and ToDisposableLinkedList will eagerly call GetEnumerator.

And with this behavior, the code below will throw (it doesn't in #726):

var sequenceA = Enumerable.Range(1, 1);
var sequenceB = new BreakingSequence<int>();
sequenceA.Interleave(sequenceB).Take(1).Consume();

Is it acceptable ?

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

No branches or pull requests

2 participants