Skip to content
This repository has been archived by the owner on Aug 1, 2024. It is now read-only.

Missing syncronous methods for dotnet core? #367

Closed
tidyui opened this issue Nov 29, 2016 · 29 comments
Closed

Missing syncronous methods for dotnet core? #367

tidyui opened this issue Nov 29, 2016 · 29 comments

Comments

@tidyui
Copy link

tidyui commented Nov 29, 2016

This might be a stupid question, but after adding version 7.2.1 to my dotnet core 1.1 netstandard 1.6.1 project there are only async methods available even though all "getting started" guides clearly contains syncronous versions of the methods.

Am I missing here or have they not been implemented yet for dotnet core?

Best regards

Håkan

@erezvani1529
Copy link
Contributor

erezvani1529 commented Nov 29, 2016

Hi @tidyui,

Thanks for bringing this up. You are right. Our NetCore/Netstandard support does not yet include Sync implementation of the APIs.
Please note that this change is on our Roadmap as one of the parity items for the transition to .NetCore and we will update with more specific timelines as soon as it is available.

Thanks!

@tfannon
Copy link

tfannon commented Jan 3, 2017

@tidyui

Were you able to continue developing in .net core with the asynchronous calls? I ported the sample over and it appears to be working in the simplest case. I was wondering if you ran into any other problems.

Thank you,
Tommy

@erezvani1529 erezvani1529 modified the milestone: 9.0 Jan 17, 2017
@TechInceptions
Copy link

I wonder if there's any update on this.
Libraries that depend on WidowsAzure.Storage currently have to "fake" the synchronous calls.

@tidyui
Copy link
Author

tidyui commented May 9, 2017

I simply decided to not support Azure storage until the implementation is complete:)

@pemari-msft
Copy link
Member

Hi @TechInceptions , @tidyui
Can you clarify for us what your need is for the synchronous methods? Overall, sync is much less resource efficient than async and is not something that we recommend to our customers. You can always use .GetAwaiter.GetResult() if you need to block on the async calls.

We have had some conversations with the CLR team, who provided this guidance:

If you expose an asynchronous endpoint from your library, avoid exposing a synchronous method that just wraps the asynchronous implementation. Doing so hides from the consumer the true nature of the implementation, and it should be left up to the consumer how they want to consume the implementation. If the consumer chooses to block waiting for the asynchronous implementation to complete, that’s up to the caller, and they can do so with their eyes wide open.
This is even more important for the “sync over async” case than it is for the “async over sync” case, because for “sync over async,” it can lead to significant problems with the application, such as hangs.

.NET Core team has chosen to not support a true sync api for the reasons listed above (resource consumption, etc.) Even if we implement a true sync, it would end up being sync over async at some level. For this reason, we believe that adding fake sync apis would be a hindrance and not a help to our customers. Please let us know if you have any feedback regarding this.

@erezvani1529
Copy link
Contributor

One instance of.NET Core team has chosen to not support a true sync api for the reasons listed above (resource consumption, etc.) Even if we implement a true sync, it would end up being sync over async at some level.

Here is one example in HttpWebRequest implementation where the provided sync methods are not true sync but in fact sync over async(Note that HttpClient has never supported sync).

@tidyui
Copy link
Author

tidyui commented May 12, 2017

Well basically I just want to create a storage provider implementing a given interface which is synchronous. Since the documentation previously stated that this was supported I just waited for it to be implemented, but if it's not going to be available in .NET Core I'll just block the async calls from within the Interface implementation.

Regards

@TechInceptions
Copy link

You should support true sync because millions of operations, as coded in people's libraries, ARE synchronous (for all kinds of reasons).

Regarding synchronicity, Ideally, APIs should have two paths: True Sync, and True Async.

In the .NET world, "sync over async" is filled with anxiety - one never knows how it's going to turnout.
Taking your library as an example, when you yanked the sync methods from the API surface, the 1st "patch" implementations will most likely be "sync over async".

Regarding the ".GetAwaiter.GetResult()" work around, is it more reliable that "Task.Run (()=> ... )" ?

I suspect ".GetAwaiter.GetResult()" will lead to deadlocks in UI chained scenarios.

If the consumer chooses to block waiting for the asynchronous implementation to complete, that’s up to the caller, and they can do so with their eyes wide open.

I have heard that argument before and I don't buy it. Isn't it better that the people with a better understanding of their own codebase do the tricky work of ensuring that something like "sync over async" works?

Mark my words, as we try to create a "better .NET" by forcing async, we are actually creating a "worse .NET" by generating "sync over async" as a direct consequence.

@mirobers
Copy link
Member

Let me explain the rationale. True sync is not an option for a library if it is consuming a library that does not support true sync, as is the case for us and .NET Core. (And anyway, true sync is something of a misnomer for APIs that make network calls, but that's a different discussion.) So we have two options: create a sync-over-async wrapper, or let the caller do it. But as we have seen, the official guidance and prevailing wisdom is that exposing a wrapper should be avoided. This is with good reason, and it's similar to avoiding an async API that wraps a synchronous invocation. Avoiding such APIs not only keeps the API surface clean, it also avoids confusion. Customers will think there is true support for sync or async and expose this in their own public interface only to discover later that the underlying implementation doesn't actually support it! So we have concluded that it is best to follow the prevailing wisdom in this case.

You are correct that deadlock avoidance is essential, especially in sync-over-async scenarios, so let me say a little about how this is done. The deadlock issue arises when a thread calls an async method and then blocks waiting for the result, while the async method chain is waiting for the thread to free up so it can continue. The solution is to have the async method do its work in a different context where it won't be constrained by a limited pool of threads. This is done internally in the async method using ConfigureAwait, so deadlocks are avoided within the library itself.

If you feel that true sync is important then you can bring this up at https://github.com/dotnet/corefx to get this supported in .NET Core.

@moldovans
Copy link

moldovans commented Jul 12, 2017

The sync is not an option. Very well. But imagine I am a beginner and there's a few documentation about Azure Storage and the usage of the synchronous methods. How should I guess on the usage of Async methods?

Microsoft release the libraries without properly documenting and explaining it :

There is any MS documentation on how to use Azure Tables with ASP.NET Core.

Look what kind of "documentation" has Microsoft on async methods: looks like some ugly auto-generated text without any example.

@isaacabraham
Copy link

Please reconsider this - it's making our porting over from net4x to netstandard much more difficult. Sync methods also have their uses, particularly with e.g. scripts, where you want instant feedback. Having to continually force results is a pain for us and reduces readability of our code.

@danroot
Copy link

danroot commented Feb 23, 2018

Here's a concrete example where I'm bumping into this due to non-async .NET Core primitives: I would like an AzureBlobStorageFileProvider that implements IFileProvider against azure storage. I plan to use this with StaticFiles to serve them out of blob storage. IFileProvider.GetFileInfo and IFileInfo CreateReadStream are not async, therefore I can't be "async all the way". I believe GetAwaiter().GetResult() might work, but am also worried about threading issues related to that. IMO, either IFileProvider and IFileInfo should be Async or CloudBlob needs a non-async OpenRead() method.

@isaacabraham
Copy link

This also is a royal pain for the Azure Storage Type Provider - type providers run synchronously, so no matter how many async calls, we always need to get with a blocking call at the end. It's especially annoying where we currently use simple methods like ListBlobs to populate intellisense for developers but now have to replace these with the overly complex async version which provides continuation tokens between batches.

I'm not suggesting that there isn't a good use for the async methods, but removing the other ones strikes me as overly prescriptive.

@copernicus365
Copy link

copernicus365 commented Mar 12, 2018

The big problem is, this is not a new API, and the original Azure storage API did have these synchronous methods. Not only that, we all depended on that original API, and many of us wrote loads of code against that original API with syncrhronous methods, and that was often even before async had even become an official part of the platform. (Remember the Async Targeting Pack anyone?, it wasn't that long ago).

While I have zealously converted my code-bases to async as far and often as possible, there's still good parts of many parts of our code pipelines (and that is always the problem with this, conversion causes beginning to end pipeline rewrite) that make synchronous calls. So I would suggest at least a compromise here. I mean it's noble and all to use this time to strip out the old way of doing things, but it seems to me there could be a compromise that fulfills that goal while still allowing parity with the API as it has existed..

So why not provide an extra netstandard (only) library, named maybe WindowsAzure.Storage.Synchronous, that is made up of extension methods that expose the missing synchronous API calls? This way would unencumber the main library, and make it clear to anyone using the synchronous methods (which may require a using statement that even states something like using ... .Synchronous) that this is not the best way going forward. Seems like this could be done fairly quickly, and allow millions of lines of code to upgrade quickly without unreasonably strong-arming them into updating end-to-end pipelines of code when the resources may not be there to do that.

@seankearon
Copy link

If people want to port to some libs to netstandard without changing sync calls to async, is calling task.GetAwaiter().GetResult() a sensible approach? For example,

blob.Exists()

would become

blob.ExistsAsync().GetAwaiter().GetResult()

@isaacabraham
Copy link

Yes, and in F# it becomes blob.ExistsAsync() |> Async.AwaitTask |> Async.RunSynchronously. This is a pain in scripts where 9/10 times you absolutely don't care about asynchronous workflows, you want immediate feedback.

For various reasons I'm now tempted to dump the Azure SDK completely and just switch to the raw REST API, maybe using the rest codegen tool.

@seankearon
Copy link

Thanks, but it looks like I'm going to back out of this. I'm starting to find API differences other than just forcing sync, such as not finding an equivalent for CloudTable.CreateQuery.

This is a real shame as it really raises the entry requirements and makes it hard to move to .Net Core. :(

@FlorianGrimm
Copy link

I'm new to the the .Net Core thing and .Net Standard, but not to the .net Framwork. Playing with the new stuff. I need a very long time to understand, why the "same" .Net Standard Libraries works on an .Net Core app, but not on a .Net FrameWork 4.7 console. Because of an mistery "Commit" MethodNot Implemented Exception. Perfomance is a good thing. But if compile works than the runtime should not throw linker errors. Tested with WebJobs - SDK preview. #if SYNC was real show stopper.

@erezvani1529
Copy link
Contributor

Well, after enough discussion, following the lead set by HttpWebRequest we have decided to offer sync methods as sync-over-async. This is a time buffer for our users before we deprecate the sync APIs in future versions of the library.

@copernicus365 , to keep it simpler for people, particularly those who are already using the library, our Netstandard2.0 support which includes the feature parity between desktop and Netcore, will leave the methods as-is, in the same namespace / binary of the split-per-service packages(split packages are in preview at the moment). Naturally, we’ll need to echo guidance in our docs by the .NET team about the benefits of converting to async, and try to make as clear as possible that it’s sync-over-async under the covers.

Many thanks to everyone who took the time to chime in on this thread and give us feedback!

One related note - for existing people who are already using the sync methods on .NET Desktop, these will also be moving to sync-over-async. We’re modifying the codebase to use HttpClient everywhere (both in desktop and in .NET Core), which doesn’t offer any sort of true sync. This is to get away from having separate implementations of everything (one with HttpWebRequest and one with HttpClient), which has led to many bugs and behavior differences between the different implementations.

@copernicus365
Copy link

@erezvani1529 woohoo! Thank you guys for listening to our feedback!!!
Long live .NET

@isaacabraham
Copy link

@erezvani1529 any ideas on when this will happen?

@Mossharbor
Copy link

Mossharbor commented May 21, 2018

I started a project in GitHub to create a sync wrapper around the newer API. This should help speed up porting considerably for people that want to get up and working quickly, without having to change any existing code.

https://github.com/Mossharbor/AzureWorkArounds.Storage
https://www.nuget.org/packages/Mossharbor.AzureWorkArounds.Storage/

Install-Package Mossharbor.AzureWorkArounds.Storage

@erezvani1529
Copy link
Contributor

erezvani1529 commented May 31, 2018

Hi,

Apologies for the delay in updates. Please try the latest version of Blob, File and Queue preview packages using Netstandard2.0 target and let us know if you see any issues.

Please make sure to review the BreakingChanges lists on the nuget packages before using them.

Thanks,
Elham

@seankearon
Copy link

Oh, this is awesome. I can start porting some older code over now. Thank you very much, team!

@RobertoPrevato
Copy link

RobertoPrevato commented Jun 21, 2018

I don't understand how the mentioned CloudTable.CreateQuery fits into the sync/async discussion. This method seems to be CPU bound to me (not needing async version), why was it removed from Net Core implementation?

Indeed, I found this issue related to CloudTable.CreateQuery method: #491.

@amshekar
Copy link

amshekar commented Jul 11, 2018

@seankearon
Is there any difference if we use below syntax
task.Result Over
task.GetAwaiter().GetResult()

@pjt33
Copy link

pjt33 commented Sep 12, 2018

Please try the latest version of Blob, File and Queue preview packages using Netstandard2.0 target and let us know if you see any issues.

What about Table?

@isaacabraham
Copy link

It's a real shame that nearly two years after this issue, it's not been possible to revert this change. We're still in the situation that the net framework version of the same version of the SDK has sync methods but the netstandard version does not (as well as various overloads of the Table Query SDK being removed).

@firebellys
Copy link

I need table api functions now, not sure why they cleaned up Blob, File and Queue and Cosmos but not Table.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests