Support for TPL & async await

[PaulNorth]

I would like to use your library to add support for Consul to Microsoft Orleans (https://github.com/dotnet/orleans) but the lack of async support is a big issue for that project.

I see there is a PR which has switched the client code to use HttpClient, do you intend to build on this and move to supporting async requests?

Thanks for the work so far!

[highlyunavailable]

Do you mean make the rest of the calls async (e.g. Client.KV.Put)? The reason that most of them are sync is because A. when I wrote it the first time, it was a heck of a lot easier to set up that way. and B. all the Consul Go API calls are sync, and this was mostly just trying to be a copy of the Go API but in C#.

I'd probably make a separate namespace like Consul.Async unless you have a better idea - I don't want to modify the existing signatures to be async, since that breaks every downstream consumer of this package, and I don't want to add *Async (e.g. Client.KV.PutAsync, ListAsync etc.) copies of every method and bloat the main interface.

The major reason I re-wrote the client to use HttpClient is someone wanted DNXCore50 support and WebRequest/WebResponse doesn't exist in .NET Core.

I'm very much open to discussion about this, so please let me know what exactly you mean by "async support".

[highlyunavailable]

Also FYI, creating an async client should be trivial. It's literally just copying the normal client interfaces, adding the async keyword to each method and replacing the call to Execute() in each method with ExecuteAsync(). I'll give it a shot today.

[PaulNorth]

Thanks @highlyunavailable that's great news.

To clarify I did mean using the *Async methods on HttpClient, adding the async/await keywords and returning Task or Task<T> from all async methods as per this example https://msdn.microsoft.com/en-us/library/hh191443.aspx#Anchor_9

Thanks again.

[highlyunavailable]

Yeah, okay. I already sort of do that:
https://github.com/PlayFab/consuldotnet/blob/develop/Consul/Client.cs#L524-L529

Each other call (KV.Put etc.) is using that Execute() method which just waits for the async to complete. It's sort of a fake sync API.

It feels cleaner to me to make a Consul.Async namespace with a ConsulAsyncClient class that implements async from the get-go and then chain new copies of the methods off of it rather than try to hang Sync and Async methods off of the same class, plus it means I have interface freedom for the new class.

Unless you have any strong objections to this method, the above is what I'm going to do.

[highlyunavailable]

Well, I found one strong objection: it's a huge pain to do it that way. I see why we have 5 billion DoodlyDoAsync methods in the TPL etc. It probably does make more sense to make an interface to just hang these off of and then implement it on the existing classes.

[PaulNorth]

"It probably does make more sense to make an interface to just hang these off of and then implement it on the existing classes."

I agree.

[highlyunavailable]

I'm just trying to figure out a good way to do this - the problem is such:

https://github.com/PlayFab/consuldotnet/blob/master/Consul/ACL.cs#L336-L360

Note how in that chunk of code, ACL is of type IACLEndpoint.

Unless I want to expand the existing interface IACLEndpoint to include CreateAsync etc. which I would rather not (since I know people write unit tests against a mocked version of that interface), I'm going to need some other way to attach the class to the Client object.

This is kind of why I wanted a class specifically for async, since I've sort of painted myself into a corner with the interfaces - either I change the interface, or I have to expose this functionality some other way.

[PaulNorth]

I may have misunderstood the problem, but it doesn't look like that code is making any IO bound calls so it doesn't need an Async version?

Also it is best where possible to use the await keyword rather than GetAwaiter().GetResult(). There is a good explanation of why here: http://blog.stephencleary.com/2014/12/a-tour-of-task-part-6-results.html

Apologies but I will be AFK from now for the evening.

[highlyunavailable]

Yeah, I'll figure it out. I'll have something ready for you to take a look at to see if it's something you'd like to use by the time you come back.

[sergeybykov]

Why not go more radical - only expose async APIs, and document how to call them synchronously?

[highlyunavailable]

@sergeybykov because this library was originally created as a sync library, so either I make an Async version or I break compatibility for everyone that was using it before I rewrote it to be all-Async.

It's not a bad idea though, and it would let me clean up some of the uglier interfaces, and since old versions are still in nuget, it won't really break people's code unless they want to update their copy of the library to use new Consul functionality.

What is the best way to call an async API synchronously anyway? I always hear "don't use GetAwaiter().GetResult() but that seems like the best option if your method isn't already async (which means you can't use await). I'm not really a C# developer by trade, so I don't know all the best ways to do TPL stuff.

[shayhatsor]

@highlyunavailable, I see that you need some advice on async stuff, so here goes:
I had the same issues when I implemented my .NET ZooKeeper client. Like you, I wanted to provide both sync and async APIs, but that was a bad idea. So I converted all blocking code to async. Now I was faced with the question of whether I should be using ConfigureAwait(false) in my code. After reading this and this, i made sure every await in my code ended with ConfigureAwait(false). Done, but now how should people call my async methods in a blocking fashion? however they like.
In conclusion : as long as your library is async all the way and every await ends with ConfigureAwait(false) you're supporting both async and sync APIs.

[highlyunavailable]

@shayhatsor thanks for the advice. The whole ConfigureAwait(false) thing feels like cargo cult coding (despite it being official guidance, of course) to add to EVERY await, but I'm used to more sane Task style libraries (mostly Go) so the entire TPL feels quite jarring to me.

@sergeybykov suggested a way where I provide both sync and async versions (e.g. there will be client.KV.Get() (sync) and client.KVAsync.GetAsync() (async)). Under the hood, the sync versions just call the async versions and block with GetAwaiter().GetResult().

I'd like to rewrite the whole thing to be async but I know most of the current users consume the Sync APIs. Maybe the right answer is just to say "Hey we changed everything by making it async, if you need sync just add GetAwaiter().GetResult() to your code or make your stuff async and await properly. Peace out."

[shayhatsor]

The whole ConfigureAwait(false) thing feels like cargo cult coding (despite it being official guidance, of course) to add to EVERY await, but I'm used to more sane Task style libraries (mostly Go) so the entire TPL feels quite jarring to me.

I know... it's annoying. the best part of it is that if you forget it even once, your sync users will get a nice deadlock.

@sergeybykov suggested a way where I provide both sync and async versions (e.g. there will be client.KV.Get() (sync) and client.KVAsync.GetAsync() (async)). Under the hood, the sync versions just call the async versions and block with GetAwaiter().GetResult() .

you can do that, it's ugly but will work.

I'd like to rewrite the whole thing to be async but I know most of the current users consume the Sync APIs. Maybe the right answer is just to say "Hey we changed everything by making it async, if you need sync just add GetAwaiter().GetResult() to your code or make your stuff async and await properly. Peace out."

Even if you decide to currently provide both sync and async APIs you still must:

1. convert all your library to async except only the public surface sync methods. this is because having any blocking code called from an async method effectively makes the whole call chain not purely async. don't go there.
2. use ConfigureAwait(false) everywhere. if you don't, users will get deadlocks when they call your code synchronously from anything that has a synchronization context e.g. GUI or ASP.NET.

[sergeybykov]

Another path to consider is add async methods now, mark sync methods as deprecated (now or later), remove sync methods in a few releases.

[highlyunavailable]

I'm going to just make it fully async. The reason is I'm already introducing breaking changes (Client renamed to ConsulClient) in the next release and I'm converting to use the HttpClient classes instead of WebRequest/WebResponse and XUnit from MSTest, so there are already plenty of changes that will cause havoc in other small ways, plus I'm not confident that I won't cause deadlocks with my half-baked use of HttpClient so I may as well do this the right way and just make a clean break between the releases.

[highlyunavailable]

Thanks for your help, everyone.

I've got a fully async client here:
https://github.com/PlayFab/consuldotnet/tree/feature/asyncclient

Nuget packages:
https://ci.appveyor.com/project/highlyunavailable/consuldotnet/build/0.6.1.131-feature/asyncclient/artifacts

Could you take a quick look to see if it's what you need @PaulNorth ?

Note: Somethings are still not going to be "truly" async - for example, Locks and Semaphores use GetAwaiter().GetResult() because you can't await in a lock {} block - same for anything in finally {} blocks. All the primitives (K/V store, Sessions, etc.) are async though.

If it's what you're looking for then I'll update the readme with big warning signs and roll out a release.

[shayhatsor]

Note: Somethings are still not going to be "truly" async - for example, Locks and Semaphores use GetAwaiter().GetResult() because you can't await in a lock {} block - same for anything in finally {} blocks. All the primitives (K/V store, Sessions, etc.) are async though.

@highlyunavailable, there are async versions of those:

1. semaphore - use SemaphoreSlim.WaitAsync
2. lock - use AsyncLock.

about awaiting in finally blocks, you should use C# 6. AFAIK this is orthogonal to the .NET version.
if going the C# 6 route isn't an option, you can do this which is a good solution.

[PaulNorth]

Great work everybody! I will pick this up today and include it in the improvements I am making to the Consul-Orleans Membership Provider.

[highlyunavailable]

@shayhatsor I mean the Consul kind of Lock/Semaphore - the distributed ones that use Consul's consistent state to allow multiple machines to contend for a K/V in Consul, not the C# kind.

I noticed that await in finally was working on my machine (VS2015) but not on Appveyor, so it sounds like I need to upgrade the CI box to use C# 6 and that problem will be fixed.

@PaulNorth please note that this isn't released to nuget, I'd just like you to test it and make sure it's what you need and that there are no surprises before I do make a real release. There was plenty of redesign so it's entirely possible that I introduced a new bug somewhere. I'm fairly confident I didn't since all the tests pass, but I'd like to have someone try it at least.

[shayhatsor]

@shayhatsor I mean the Consul kind of Lock/Semaphore - the distributed ones that use Consul's consistent state to allow multiple machines to contend for a K/V in Consul, not the C# kind.

ok, but you noted:

you can't await in a lock {} block

Your code can still be fully async. as an example I've looked at your lock implementation.

1. Replace all locks on _lock with AsyncLock on _lock.
2. Keep the regular locks on _heldLock, as these provide freshness to the value. You can also use
   Fenced which provides freshness without acquiring a lock (this is just a super optimization, basically because i'm crazy)
3. follow the async all the way rule to the public surface methods.

[PaulNorth]

Having switched to the new async build the methods we are using all appear to be working just fine. Our usage is by no means an exhaustive test; we are only using KW.Put, KW.Get, KW.CAS, Agent.ServiceRegister, Agent.ServiceDeregister & Catalog.Service.

The migration was pretty painless and I can only imagine your existing clients will be grateful to have a purely async interface.

Cheers!

[highlyunavailable]

@shayhatsor Okay, fair enough, I can replace much of the lock {} blocks with AsyncLock, it seems to work fine, but I'm also unsure of what you mean by your earlier statement "follow the async all the way rule to the public surface methods." in relation to what you said before: "1) convert all your library to async except only the public surface sync methods. "

Is this saying it's fine if Acquire(), Release() etc are not async and use GetAwaiter().GetResult() as long as all the calls below that in the call chain are async? If so, then this is the case now - it's completely async except for those surface methods Acquire, Release, Destroy. It also seems weird to have lock-style statements be async since they're supposed to be guarding critical sections, so it feels like you want blocking here.

One final thing I'm struggling with is AutoSemaphore (and AutoLock) - they're a pair of classes that are supposed to let you do using (new AutoLock(lockargs)) {something} and it will do an I/O to Consul, get a lock there, do your thing in the using block, and then the Dispose method does another I/O to unlock.

Bad practice due to long constructor/disposer calls, I suppose, but it's in there now, and I don't see a good way of doing async acquire/release in the constructor and disposer without GetAwaiter().GetResult(). I guess I could deprecate the class, I don't know anyone that is using it.

@PaulNorth Sounds good - I'll cut a release and any further modifications (e.g. the discussion above) will be new releases. Thanks for putting in this issue!

[shayhatsor]

I'm also unsure of what you mean by your earlier statement "follow the async all the way rule to the public surface methods." in relation to what you said before: "1) convert all your library to async except only the public surface sync methods. "
Is this saying it's fine if Acquire(), Release() etc are not async and use GetAwaiter().GetResult() as long as all the calls below that in the call chain are async?

ok, i'll explain again, remember I wrote this:

1. convert all your library to async except only the public surface sync methods. this is because having any blocking code called from an async method effectively makes the whole call chain not purely async. don't go there.

GetAwaiter().GetResult() is a blocking call. you don't want to block the thread while you're waiting for network response. just use await instead . except as explained here

It also seems weird to have lock-style statements be async since they're supposed to be guarding critical sections, so it feels like you want blocking here.

it is weird, but it's what you want. note that you can't await in a normal lock because it is owned by the running thread, which you actually release upon await... so it doesn't really make sense. Instead, you need something that isn't thread related that provides mutual exclusion asynchronously. this is what AsyncLock gives you. Keep in mind that AsyncLock isn't reentant and normal lock is. but I don't see it as a requirement here.

One final thing I'm struggling with is AutoSemaphore (and AutoLock) - they're a pair of classes that are supposed to let you do using (new AutoLock(lockargs)) {something} and it will do an I/O to Consul, get a lock there, do your thing in the using block, and then the Dispose method does another I/O to unlock.

what you do is this:

• use async factory method instead of the constructor. (make the ctor private)
• use async dispose method instead of the dispose.
  this does mean you can't use using anymore, as c# doesn't have an async using yet.
  but you can make your own. I actually did that in zookeeper. and an example of usage is here

[shayhatsor]

just noticed I haven't explained this:

I'm also unsure of what you mean by your earlier statement "follow the async all the way rule to the public surface methods." in relation to what you said before: "1) convert all your library to async except only the public surface sync methods. "

I'll explain by example. let's say you had your sync class:

public class fooAPI
{
    int Foo1()
    {
    //some blocking code goes here
    }
}

and now you're going async, but wish to also keep the sync methods. you'd do this:

public class fooAPI
{
    int Foo1() 
    {
      //this is OK because it's a public surface method that only calls the async code.
      //it isn't part of any async flow, it's here just to keep the sync interface.
      return Foo1Async.GetAwaiter().GetResult();    
    }

    Task<int> Foo1Async()
    {
       //there's no blocking code here or in any method called from this method.
       //every async method is called with await and ConfigureAwait(false)
    }
}

[highlyunavailable]

Awesome, the example is what I was thinking and we're both thinking the same thing.

Okay, then I'm not going to change anything further this release because that's exactly what I'm doing. Only the Lock/Semaphore stuff is sync, EVERYTHING else (the primitives in KV storage, sessions, etc.) are async. I am not putting any syncs in any async flows.

Thanks for all your help and for explaining this, everyone. I'm going to close this issue now by saying that there's a new release that's all Async except for lock/semaphore at https://www.nuget.org/packages/Consul tagged 0.6.1.0. This is the official package, @PaulNorth

I'll open a new issue for explore converting the distributed locks to fully async.

[sergeybykov]

👍

[gabikliot]

Great cross project collaboration! 4 different teams, and the async support added to Consul.Net in 2 days!