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

Automatically create non-async methods from async methods #12931

Closed
UppaJung opened this Issue Aug 4, 2016 · 13 comments

Comments

Projects
None yet
8 participants
@UppaJung
Copy link

UppaJung commented Aug 4, 2016

Developers of an class often have to write two versions of the same method to support async and non-async callers:

class Bar 
{
    // Implementation of Foo as an async method
    async Task<object> FooAsync(CancellationToken cancelToken = default<CancellationToken>)
    {
      await alpha.MailAsync(cancelToken);
      return await beta.MailAsync(cancelToken);
    }

    // A replica  of Foo as a non-async method, which is the same as the above except:
    //     1) I does not take a cancellationToken
    //     2) It cannot use async to await calls to other methods
    object Foo()
    {
      alpha.Mail()
      return beta.MailAsync(CancellationToken.None).Result;
    }
}

Issue #7769 proposes auto-generating async methods from non-async ones (Foo -> FooAsync), but this is problematic as you need more information to generate async methods than may be available in the non-async method, such as how to handle cancellation tokens.

Since all the information to generate a non-async method should be present in the async version, why not go the other way and automatically generate non-async methods from the async ones (FooAsync->Foo)? This would ensure that developers who make the effort to write a correct aync method don't have to invest extra time to write a second non-async version unless there's a genuine need to change the underlying implementation.

To convert, the compiler will need to remove the cancellation token paremeter (if it is present). The parameter is replaced with CancellationToken.None. Branches triggered on CanBeCanceled or IsCancellationRequested can be simplified as both are always false. Most code that uses the cancellationToken will be automatically removed as dead code, whereas a developer would otherwise have to do this dead-code elimination manually--possibly introducing errors.

The compiler will also need to handle instances in which the method itself calls another async method. It should replace the async call with a non-async version of that method if one is available: if a method with the same prefix before "Async" exists and it has the same interface, minus the Task<> wrapping of the return value and the cancellation token. If no non-async method exists and none can be generated, the compiler can leave the async call in place and blocks the calling thread until the asynchronous operation is complete, either by using the Task.Result property or calling Wait. Again, the process here is simple and straightforward, but if the compiler doesn't do it a human will have to and will thereafter have to work to maintain two copies of the same code.

The conversion should only happen if the code does not already contain a non-async version of the same method.

If this feature were instrumented, I'm guessing many of us who maintain an async and non-async version of the same code would remove the non-async as the same code would end up being generated, but thereafter we would only have one version to maintain.

@HaloFour

This comment has been minimized.

Copy link

HaloFour commented Aug 4, 2016

If no non-async method exists and none can be generated, the compiler can leave the async call in place and blocks the calling thread until the asynchronous operation is complete, either by using the Task.Result property or calling Wait. Again, the process here is simple and straightforward, but if the compiler doesn't do it a human will have to and will thereafter have to work to maintain two copies of the same code.

This is a massive anti-pattern and can very easily lead to deadlocks. Async has to be either all-the-way or not-at-all, trying to blend them blindly like this will break apps.

@vladd

This comment has been minimized.

Copy link

vladd commented Aug 4, 2016

There is a nice article by Stephen Toub on MSDN, explaining why having synchronous counterparts is a bad idea: Should I expose synchronous wrappers for asynchronous methods?

@svick

This comment has been minimized.

Copy link
Contributor

svick commented Aug 5, 2016

@vladd There is a difference between "synchronous counterpart" and "synchronous wrapper". This proposal basically creates synchronous counterpart when it can, and synchronous wrapper otherwise.

@HaloFour

This comment has been minimized.

Copy link

HaloFour commented Aug 5, 2016

@svick

The "otherwise" is the dangerous part. If the refactoring could strictly and correctly identify the synchronous methods that correspond to the asynchronous methods and recreate a new synchronous version that might be fine, but there should be no fallback. Even then I don't think I'd trust it to do the right thing often.

@Joe4evr

This comment has been minimized.

Copy link

Joe4evr commented Aug 5, 2016

This proposal basically creates synchronous counterpart when it can, and synchronous wrapper otherwise.

And either one is a bad idea. It should be up to the caller to call an async method and synchronously wait for it to complete, it's not the responsibility of the compiler to make synchronous alternatives.

@svick

This comment has been minimized.

Copy link
Contributor

svick commented Aug 5, 2016

To me this sounds:

  • useful: Having both async and non-async methods is a common requirement for libraries, especially older ones that have to maintain backwards compatibility. And currently, the best solution is to write lots of duplicated code, which is not ideal.
  • problematic: As others pointed out, using .Result or .Wait() can easily lead to deadlocks. Also, switching between the async and non async version might not be as simple: How do you set useAsync in the FileStream constructor correctly? How do you deal with await Task.WhenAny() or await Task.WhenAll()? Etc.

So, I think this is something that should exist, but not be built-in in the compiler. That way, each library can decide how exactly the transformation should look.

Maybe it could be a source generator (#5561)?

@HaloFour

This comment has been minimized.

Copy link

HaloFour commented Aug 5, 2016

@svick

I can agree that the manual duplication of code can be tedious and annoying. It would be nice if there was a way to automate this. But I do fear that any such tool will both occasionally get things wrong in unexpected and hard-to-diagnose ways, and that it would only really help in the most basic of cases.

Either way, it's something a custom analyzer/source-generator could likely tackle. I'd probably go with the former, have an analyzer offering a one-time fix-it to generate what it thinks is proper synchronous code but generate it right into the class so that the developer can see it, verify it and, if necessary, manually "fix" it.

@UppaJung

This comment has been minimized.

Copy link

UppaJung commented Aug 5, 2016

@svick @HaloFour

A source generator leaves you with two copies of the same code to maintain and increases the chance that a developer will mistake when keeping these two copies syncrhonized. When there's a bug, the developer fixing it will have to know to check both copies and fix both copies. When the code needs to be modified to support a new feature, the developer will have to modify both the original code and the generated code.

I had assumed that if you wanted to ensure developers could still decide whether and under what conditions the code would be automatically generated, that attributes would give developers this control.

[GenerateNonAsyncForMe]
async Task<object> FooAsync(CancellationToken cancelToken = default<CancellationToken>)

The standard attribute would lead to a compiler feature if there was an async method call that did not have a corresponding non-async method to use in its place. You could use another attribute to override that error and take the risks of a .Wait()

[GenerateNonAsyncForMeEvenIfWaitsAreNeeded]
@HaloFour

This comment has been minimized.

Copy link

HaloFour commented Aug 5, 2016

@UppaJung

Source generators are run as a part of the compilation process so the sync version would continually be recreated. That could work fine for the simplest of scenarios where the sync version doesn't need manual intervention.

But I firmly believe that an analyzer which generates a one-time-copy is the better route because I can guarantee that the developer will have to review the results and likely tweak something here or there. That's not really possible with source generators, even if the generator offers a ton of different options. Sure, the developer will have to maintain both versions, but if they had to tweak the results they would have to do that anyway. They could then simply delete the sync version, rerun the tool and make whatever tweaks they need to make.

@UppaJung

This comment has been minimized.

Copy link

UppaJung commented Aug 5, 2016

@HaloFour

You wrote "because I can guarantee that the developer will likely have to review the results and tweak something here or there."

I requested this feature because I'm a developer and find myself developing duplicate methods that don't need such tweaks. Let me provide an example of why.

I'm using the new Core EntityFramework and exposing methods that modify database structures using that framework. That framework provides abstract classes that support both sync and async methods. The SQLite support for the framework, which my UWP client code will use for local storage, is apparently inherently synchronous, but to support the async methods it does exactly what Stephen Toub does not recommend: it does some waiting that can lead to deadlocks. The SQLServer implementation of the framework, which my server code will use for its copy of the data structures, is truly async.

I believe the right thing for me to do here is to write my layer with both async and sync calls to the entity framework layer. There's really nothing to tweak here but whether I'm using sync or async when I go down to the entity framework layer. If I do that, I can call the sync methods on my client and not have to worry that it will be implementing hacks to appear asynchronous. I can call the async methods in my server code. Alas, to do this I have to write everything twice. As I look through all these replicated copies of the same code, there's nothing here that would need to be tweaked--it's all just data manipulations in a database that will either be performed synchronously or asynchronously.

Should it really be necessary for me to maintain two copies of so many methods that do exactly the same thing, with the exception of removing some syntax and cancellation tokens?

@HaloFour

This comment has been minimized.

Copy link

HaloFour commented Aug 5, 2016

@UppaJung

If that's the case I still feel that a source generator is the best route rather than trying to bake something into the language. It would create that code for you without you having to do anything, and it would do so on every compilation so you'd never be required to manually keep things in sync. My concern is that asynchrony is hard and that trying to treat asynchronous and synchronous the same way can very easily lead to some very difficult-to-find problems.

@bbarry

This comment has been minimized.

Copy link

bbarry commented Aug 9, 2016

This is certainly doable with a source generator. Check out @roji's AsyncRewriter project: https://github.com/roji/AsyncRewriter which basically does what #7769 is asking.

This is used in the PostgreSQL ADO.NET provider to implement async support: https://github.com/npgsql/npgsql

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Sep 11, 2017

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to #18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I have not moved this feature request to the csharplang repo because I don't believe it would ever rise in priority over other requests to be something we would ever do in any particular release. However, you are welcome to move discussion to the new repo if this still interests you.

@gafter gafter closed this Sep 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment