Support for async dispose #53

iridin · 2022-02-15T20:16:03Z

Updates

Updating target dotnet framework to 4.7.2 since 4.5 will run out of support in few months.
Updating target dotnet core to 6.0 since it is LTS.
Updating fake-cli to latest version.
Updating nugets used in all projects.
Defining dotnet versions in Directory.Build.props and using it across all projects.
Removing references to non-existing files in the .sln and adding references to the rest of solution items.
Updating README (compatibility with VS 2022)
Replacing TickSpec project description with reference to README (this will be used as description in the Nuget.org)

Support for DisposeAsync

Adding support for asynchronously disposable resources in TickSpec/ServiceProvider.fs. This requires either using netstandard2.1 or dotnet6.0, or importing Microsoft.Bcl.AsyncInterfaces. At first I planned to use AsyncInterfaces only for net472 target framework and increase netstandard version to 2.1, but in netstandard2.1 there is no method MarkSequencePoint on ILGenerator (used in TickSpec/ScenarioGen.fs). (According to documentation this method doesn't exist even in netstandard2.0, but somehow it still works).

mchaloupka

I am fine with the version updates you are doing, although it means that we will do a major version bump. However, that is worth it if we add new capability that is missing.

Make sure that you get green builds. One thing necessary to achieve that is to change the image in appveyor.yaml file - on windows you need to use newer image that will have .NET 6.

mchaloupka · 2022-02-15T20:59:19Z

TickSpec/ServiceProvider.fs

            instances
            |> Seq.map snd
            |> Seq.iter (function
+                | :? IAsyncDisposable as d -> disposeAsync(d)


Do you believe that this is a correct pattern? Would it make sense to create another interface for the provider to be also IAsyncDisposable and bubble it through?

That brings the question, should we implement this without allowing the scenario runner to be also Async. That could move us to support also async steps.

What do you think? What is the issue you are trying to resolve?

I have mixed feelings about introduction of DisposeAsync. On one hand a class should not have only DisposeAsync defined, but rather have both Dispose and DisposeAsync. Then the runtime itself decides what dispose method it calls, but only one of them is called in the end. On the other hand the DisposeAsync should not block and should not be used if it can't dispose the object asynchronously.

The way I suggest this to be implemented it is preferred to dispose objects asynchronously if they support it, but disposing synchronously the rest that can be disposed.

Supporting also async steps is a great idea, but probably much more complex to implement correctly. I'm open to suggestions.

We have a lot of cases where resources are asynchronously created and disposed with the following code:
interface System.IDisposable with member this.Dispose() = (backgroundTask { // Do the disposal }).Wait()

You have mentioned the exact thing I don't like about this:

On one hand a class should not have only DisposeAsync defined, but rather have both Dispose and DisposeAsync. Then the runtime itself decides what dispose method it calls, but only one of them is called in the end.

So, what is the benefit of having the support here? If the instances have also Dispose then what is the benefit of calling IAsyncDisposable interface without propagating the awaitable task out of the scenario run?

On the other hand, it seems to me that it can cause issues when we start calling async methods from synchronous method in the style fire & forget. In lots of cases, it may mean that the disposal will not be finished as the main thread is blocked. Your workaround is more correct in the terms of running the disposal on background thread and synchronously waiting on the result on the main thread.

When we will go for another major version, I think that we are fine dropping the code specific for .NET Framework. While the code emit and breakpoints is a great functionality, as we are not able to get it running on netstandard, it is becoming irrelevant. Considering that, creating two variants of methods to run steps (synchronous and asynchronous) should not be overly complication. And the asynchronous variant would be able to invoke also async steps and IAsyncDisposable on the store while the synchronous variant would not support that.

I agree that it makes no sense to add DisposeAsync support without supporting asynchronous steps. I think ideally the ServiceProvider should implement IAsyncDisposable instead of IDispose and itself be asynchronous.

Note that we had some time ago open pull request for async steps #23 but we closed it hoping for better implementation.

Regarding emit code, I think it is still a miss that we don't support debugging feature files with .NET 6, I see that F# fsi is having the same issue but they are just working on a fix dotnet/fsharp#12722, basically instead of using dynamic assemblies they use static, we can probably do the same, but I don't know how much effort it is and who and when can do that.

Regarding emit, you are correct, it is a miss that we don't support it. However, I believe that we should not block all development just because it is harder due to old way we emit code.

It feels to me, that removing support for something that works only on .NET 4 and adding support for async would be a nice improvement. And we can work on bringing the support for breakpoints afterwards.

Yes, it would be awesome if we can keep the support for the things we have or even more awesome if we can make it happen even for .NET 6. We can also revisit what are all the code parts we do emit for, ideally we should not have the code duplicated. In other words, there are lots of things we can improve but we have to choose how to bring the biggest value.

High level tangent/suggestion:

Ultimately someone looking at this repo in 6 months time is more interested in good .NET 8 support than some fantastic stuff in a video from 2009.

So, dont be afraid to consider:

make a v2 branch which archives what's in the repo

start a new world on master/main with only net6.0 support with a prominent mention of the v2 branch
2a. maybe remove old editions of test FWs examples etc

tag issues with 2.x vs 3.x milestones - i.e. any stuff about full fw / breakpoints etc can be 2.x

release stuff as 3.0.0-beta.1 etc until such time as you are finished cleaning things

Bottom line is that PRs like this (which is wonderful to see, welcome @iridin !) are much easier to do and/or likely to arrive if there is less ambiguous stuff that people need to traverse (I was going to suggest that the test FW parameterization stuff in this PR could go into a Directory.build.props but I think its better if everything is just rebooted to hardwired net6.0 etc). I would suggest that e.g. explicit FSharp.Core references can also be stripped out. Things like switching from default FSharp.Core to explicit lower level reqs can then be reinstated as PRs.

(I did this in https://github.com/jet/equinox when moving from V2 to V3, and am about to do the same dance soon - I will cut a v3 archive, do an aggressive tidy, and then start merging all the draft PRs I've been parking in there)

Please note this is just me talking - I'll scan stuff as a watcher but real decisions regarding this should be made by people who will have the time/interest to actually follow through.

I fully agree.

We need to find way how to make good progress and starting by implementing first the code without IL and removing the emit path for now feels to me as the right step. It will make implementation of async steps and async dispose much easier.

Sorry for taking so long to respond. I'm experimenting with making async tests all the way from testing framework into TickSpec steps. I will get back here once I have something that can be presented or discussed. Thanks

mchaloupka · 2022-02-15T21:03:43Z

TickSpec/TickSpec.fsproj

-      When, Then. Easily execute the behaviour against matching F# 'ticked' methods 
-      (let ``tick method`` () = true) or attributed C# or F# methods.
-    </Description>
+    <PackageReadmeFile>README.md</PackageReadmeFile>


mchaloupka · 2022-02-15T21:04:15Z

TickSpec/TickSpec.fsproj

+    <PackageReadmeFile>README.md</PackageReadmeFile>
  </PropertyGroup>
+  <ItemGroup>
+      <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" />


Can you add this only when the target is net framework and not netstandard? I believe, that it makes sense to move it to netstandard 2.1. If there are any build issues then we can resolve them. I believe that there are just some compilation flags that caused compilation of code that should not be compiled for netstandard.

i used AsyncInterfaces conditionally for dotnet framework only. You were right the compilation of some parts is flagged to be excluded for netstandard.

mchaloupka · 2022-02-15T21:06:10Z

README.md

- The binary should work cleanly on any .NET Standard 2.0, .NET 4.5 or later environment.
- The TickSpec solution file works with Visual Studio 2017. 
+- The binary should work cleanly on any .NET Standard 2.0, .NET 4.7.2 or later environment.
+- The TickSpec solution file works with Visual Studio 2017 and newer (tested with VS 2022).


Just change this to Visual Studio 2022, we don't need to list all supported versions. Especially when we are not sure whether it still works in the old one (I guess not).

Agree https://devblogs.microsoft.com/visualstudio/support-ends-for-older-versions-of-visual-studio-feb2022/
Can also mention Rider because it always Just Works
If someone can check it with Ionide/VSCode, that's welcome to mention too

I have changed the VS version only.

ds185357 added 5 commits February 15, 2022 17:57

Increasing target framework versions.

efc7fc4

Adding support for asynchronously disposable resources.

07f1970

Updating nuget packages.

ae36d11

Updated README.

aea897f

Replacing TickSpec project description by Package readme file reference.

73318a1

mchaloupka requested changes Feb 15, 2022

View reviewed changes

DS185357 added 3 commits February 16, 2022 09:36

Using new windows image for the build.

0f44f31

Increasing netstandard version to 2.1

b9c53f6

Updated versions in README.

5e3beee

Support for async dispose #53

Are you sure you want to change the base?

Support for async dispose #53

Uh oh!

Conversation

iridin commented Feb 15, 2022

Updates

Support for DisposeAsync

Uh oh!

mchaloupka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartelink Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bartelink Feb 16, 2022 •

edited

Loading