STJ: Dispose enumerator on exception

[manandre]

Fixes #50851

[eiriktsarpalis]

It's been a while since I last visited this problem, but IIRC fixing the issue is not as simple as proposed in this PR:

1. It needs to take into account the state machines used in async serialization. See https://github.com/dotnet/runtime/pull/50778/files#diff-65021de09c4fb9b39607f1f3ed228e53cd04b483880f66b0ea56310f71a5e400R131-R137 for hints on how this was done for IAsyncEnumerable.
2. The serializer is now making recursive calls within a try/finally clause in the serialization hot path. There is a good chance that this could regress performance so it would need to be benchmarked using the suite defined in the dotnet/performance repo.
3. The change doesn't handle other enumerable converters such as the one for dictionary types.

[eiriktsarpalis]

The implementation looks good to me now, however my concerns about this potentially regressing performance because we're adding a try/catch in the hot path still remain. Performance of the serializer has historically been very sensitive to changes made in this location, so I'd want to see a run of the benchmark suite against the change before I could sign off on it.

Given that this change is made to address a hypothetical bug that nobody has actually has reported (Disposable IEnumerators are largely legacy and if they do exist chances are users would be better off using IAsyncEnumerable<T> which does handle disposal correctly).

I'm going to close this PR for now and the related issue, thank you for taking the time to work on this!

[stephentoub]

Disposable IEnumerators are largely legacy

Not disputing the rest of your comment, but can you elaborate on this? Any C# iterator with a try/finally is going to have a meaningful IDisposable.Dispose.

[eiriktsarpalis]

I wasn't aware of that use case. It definitely adds more points in favor of us doing this, but we still need to run the benchmarks against the change.

[eiriktsarpalis]

Ok, let's see if we can get this merged. I rebased the branch and pushed a change the removes the need for new try/catch clauses in the hot path. I ran the microbenchmark suite and detected no change in performance.