rsdn / CodeJam

Set of handy reusable .NET components that can simplify your daily work and save your time when you copy and paste your favorite helper methods and classes from one project to another
MIT License
258 stars 35 forks source link

Fix disposable extensions bugs and add relevant tests. #157

Closed TheSquidCombatant closed 6 months ago

TheSquidCombatant commented 6 months ago
  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 commented 6 months ago

Additional explanations and motivation

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

https://github.com/rsdn/CodeJam/pull/157/commits/c1a000125b46302c0bff6242961621a2118efc5c#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L75

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

https://github.com/rsdn/CodeJam/pull/157/commits/c1a000125b46302c0bff6242961621a2118efc5c#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L33

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

https://github.com/rsdn/CodeJam/pull/157/commits/c1a000125b46302c0bff6242961621a2118efc5c#diff-138399ef9f53853c4f235de6ca89285ad9c2e84da0a4d8443847a466c19fbeb1R9

TheSquidCombatant commented 6 months ago

Additional refactoring and improvements

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

https://github.com/rsdn/CodeJam/pull/157/commits/ca46ce8a17d5f42dc8215230d0440c177dc15674#diff-138399ef9f53853c4f235de6ca89285ad9c2e84da0a4d8443847a466c19fbeb1R64

  1. Removed an unnecessary preprocessor conditional compilation directive.

https://github.com/rsdn/CodeJam/pull/157/commits/ca46ce8a17d5f42dc8215230d0440c177dc15674#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242L3

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

https://github.com/rsdn/CodeJam/pull/157/commits/ca46ce8a17d5f42dc8215230d0440c177dc15674#diff-f65ce38674f9332658a3ed2539215c062c082d789fae54abf0dd15cfcba27242R69

andrewvk commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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.