Fix disposable extensions bugs and add relevant tests.

[TheSquidCombatant]

1. Fixed error collection in DisposeAll method.
2. Fixed thread blocking in DisposeAsync method.
3. Added related tests in DisposableExtensionsTests class.
4. Updated LangVersion option for use new csharp features.

[TheSquidCombatant]

Additional explanations and motivation

1. If I wanted a synchronous call, then I would call the method Dispose directly, right?

c1a0001#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L75

2. All exception objects will be lost here except the last one, obviously this is not the expected behavior.

c1a0001#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L33

3. Updated the language version to use file scoped namespaces here.

c1a0001#diff-138399ef9f53853c4f235de6ca89285ad9c2e84da0a4d8443847a466c19fbeb1R9

[TheSquidCombatant]

Additional refactoring and improvements

1. Added another test for DisposeAsync method to check Dispose call itself.

ca46ce8#diff-138399ef9f53853c4f235de6ca89285ad9c2e84da0a4d8443847a466c19fbeb1R64

2. Removed an unnecessary preprocessor conditional compilation directive.

ca46ce8#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L3

3. Made awaitable DisposeAsync method, which should not break backward compatibility.

ca46ce8#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242R69

[ig-sinicyn]

Thanks for the PR, LGTM !:)

[NN---]

What is the reason for making ValueTask here?
Isn't await is enough ?

P.S.
Don't we need ConfigureAwaitable(false) as a library here ?

[ig-sinicyn]

As for the ValueTask - these two implementations have almost tame behavior so both look good for me.

ConfigureAwaitable is not used in the lib anyways. We may add the calls but I'm not sure if it should be done,

[andrewvk]

1. +1
2. At the moment, a lot of CJ code does not contain CfgAwait(false). If we move away from this, we should fix the whole library code.

[andrewvk]

The main idea behind this method is to call DisposeAsync if it is supported. And if it is not - call Dispose synchronously. The method, like much of CJ, is basically sugar, allowing you to avoid writing checks in the main code.
Creating a separate thread and executing synchronous Dispose in it is a significant change in behavior, which is not so expected. For instance, specific Dispose, can get into the database with an external connection, and launching such code will cause a crash.
As it seems to me, a method with such behavior should be different, and its name should somehow reflect the fact that it will generate a separate thread for synchronous methods.

[TheSquidCombatant]

The main idea behind this method is to call DisposeAsync if it is supported. And if it is not - call Dispose synchronously. The method, like much of CJ, is basically sugar, allowing you to avoid writing checks in the main code. Creating a separate thread and executing synchronous Dispose in it is a significant change in behavior, which is not so expected. For instance, specific Dispose, can get into the database with an external connection, and launching such code will cause a crash. As it seems to me, a method with such behavior should be different, and its name should somehow reflect the fact that it will generate a separate thread for synchronous methods.

@andrewvk, in my humble opinion, the Async suffix is a sufficient indicator to the caller, that the method body can be executed in another thread. According to the official documentation from Microsoft: "An async method provides a convenient way to do potentially long-running work without blocking the caller's thread." So, It seems to me, that the new behavior of the method is more consistent with the expectations according to its signature.

[andrewvk]

@andrewvk, in my humble opinion, the Async suffix is a sufficient indicator to the caller, that the method body can be executed in another thread.

Of course not. Asynchronous methods by default use the current synchronization context, which can be a little bit different (and that's why you sometimes need to insert ConfigureAwait(false)). Moreover, the official recommendations do not recommend using CfgAwait(false) in methods that take delegates as input, because the one who passes code expects that this code will be executed in the context in which the method with this parameter is called.
So when we pass an object with synchronous methods in there, we don't usually expect those methods to be implicitly called in another thread.

[TheSquidCombatant]

@andrewvk, in my humble opinion, the Async suffix is a sufficient indicator to the caller, that the method body can be executed in another thread.

Of course not. Asynchronous methods by default use the current synchronization context, which can be a little bit different (and that's why you sometimes need to insert ConfigureAwait(false)). Moreover, the official recommendations do not recommend using CfgAwait(false) in methods that take delegates as input, because the one who passes code expects that this code will be executed in the context in which the method with this parameter is called. So when we pass an object with synchronous methods in there, we don't usually expect those methods to be implicitly called in another thread.

We pass to the asynchronous DisposeAsync method one instance of the IDisposable interface, which has a single Dispose method. Attention, question! What should be executed asynchronously in this situation?

Casting to the IAsyncDisposable interface does not count. Because if the caller had an instance of the IAsyncDisposable interface, then the asynchronous DisposeAsync method would be called directly on it. And if we wanted to call the synchronous Dispose method, we would also call it directly on the existing instance of the IDisposable interface.

[andrewvk]

I think, I need to explain in more details the scenario for which this method was intended. There are situations when we have a collection of IDisposable (IAsyncDisposable has appeared relatively recently, and its adoption rate is not very high). However, some elements of the collection can still implement IAsyncDIsposable (but statically we don't know it!). This method allows you to run DisposeAsync, if any, without executing thread waiting for result. There was no idea to run regular Dispose in a separate thread, just as there was no idea to run DisposeAsync outside the current synchronization context.
The variant in the current MR has a much more complicated and non-obvious semantics (source code knowledge required to understand what it do). That's why, I think. it makes sense to add another method with a more descripting name, for example DisposeInParallel.

[TheSquidCombatant]

I think, I need to explain in more details the scenario for which this method was intended. There are situations when we have a collection of IDisposable (IAsyncDisposable has appeared relatively recently, and its adoption rate is not very high). However, some elements of the collection can still implement IAsyncDIsposable (but statically we don't know it!). This method allows you to run DisposeAsync, if any, without executing thread waiting for result. There was no idea to run regular Dispose in a separate thread, just as there was no idea to run DisposeAsync outside the current synchronization context. The variant in the current MR has a much more complicated and non-obvious semantics (source code knowledge required to understand what it do). That's why, I think. it makes sense to add another method with a more descripting name, for example DisposeInParallel.

It was worth writing about this in the comment to DisposeAsync method with the old behavior and creating a corresponding test. From the outside, judging by the method signature, the Dispose method itself is expected to execute asynchronously. Perhaps, mentioned method with the old behavior should have been called something like DisposeIfAvailableAsync.