Async not used correctly

[NicolasDorier]

Hey, I am the main dev of https://github.com/btcpayserver/btcpayserver a self hosted payment server which run your own node and support multiple shitcoin on top of bitcoin.

I want to use your API to provide "auto dumping" feature to merchants.

However, it seems that you are using async incorrectly by wrapping sync call with Task.Run.

What you should do is the reverse, using async/await all along the way and use YourMethodAsync().GetAwaiter().GetResult() to make synchronous calls methods.

If I propose a PR fixing all of this, will you merge?

[jjxtra]

Of course, thanks for the suggestion!

Jeff Johnson CEO / CTO, Digital Ruby, LLC

On Mon, Apr 9, 2018 at 9:03 AM, Nicolas Dorier notifications@github.com wrote:

Hey, I am the main dev of https://github.com/btcpayserver/btcpayserver a self hosted payment server which run your own node and support multiple shitcoin on top of bitcoin.

I want to use your API to provide "auto dumping" feature to merchants.

However, it seems that you are using async incorrectly by wrapping sync call with Task.Run.

What you should do is the reverse, using async/await all along the way and use YourMethodAsync().GetAwaiter().GetResult() to make synchronous calls methods.

If I propose a PR fixing all of this, will you merge?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNoGOogdVkmkdFEkxDXCfAttZoHNxTHks5tm3hGgaJpZM4TMs9P .

[bichuga]

Nicolas, do you have any more info about why what you propose is better? I've seen ExchangeSharp's pattern in training videos.

On Mon, Apr 9, 2018 at 8:04 AM Jeff Johnson notifications@github.com wrote:

Of course, thanks for the suggestion!

Jeff Johnson CEO / CTO, Digital Ruby, LLC

On Mon, Apr 9, 2018 at 9:03 AM, Nicolas Dorier notifications@github.com wrote:

Hey, I am the main dev of https://github.com/btcpayserver/btcpayserver a self hosted payment server which run your own node and support multiple shitcoin on top of bitcoin.

I want to use your API to provide "auto dumping" feature to merchants.

However, it seems that you are using async incorrectly by wrapping sync call with Task.Run.

What you should do is the reverse, using async/await all along the way and use YourMethodAsync().GetAwaiter().GetResult() to make synchronous calls methods.

If I propose a PR fixing all of this, will you merge?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93, or mute the thread < https://github.com/notifications/unsubscribe-auth/ABNoGOogdVkmkdFEkxDXCfAttZoHNxTHks5tm3hGgaJpZM4TMs9P

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93#issuecomment-379784315, or mute the thread https://github.com/notifications/unsubscribe-auth/AI8DiaUU-VBg_tQ2A0xS-9ZtGyRqs5Fxks5tm3iBgaJpZM4TMs9P .

[atp4al]

@bichuga

By calling Task.Run the code becomes asynchronous so the end effect is the same. You can say that when you write "await ..." it is as if you run the code in Task.Run and then everything after the await keyword will be done after, as if you have written it in ContinueWith.

However, it is not the case under the hood as async/await generate a state machine. (you can watch the last videos of this course for a good explanation of that). The "magic" is that there is no task created, which means there will not be a thread used. As a result, async/await will scale better.

async is good on servers because asynchronous operations scale better than threads. But if you're using Task.Run, then you're using a thread anyway. Source

See this post for a more detailled explanation about the fact there is no thread involved.

[jjxtra]

Still happy to merge the pull request in, read a few Microsoft blurbs about it, seems like a good idea.

[NicolasDorier]

So the issue is, if you do this way, then you are essentially blocking a thread when making your request, which is a scarse ressource. Task.Run is relying on the thread pool and if no more thread are available, even the async version would , counter-intuitively, block.

And this is worse! if what is called inside Task.Run is also using Task.Run this can lead to a complete deadlock once no thread in the threadpool are available. So now you have a situation where your server might experience complete deadlock/shutdown, impossible to debug, if the downstream service takes too much time to reply.

Using async/await all the way ensure that threads are freed to do other work as soon as the request is sent. This remove any deadlock risk, and keep your server fit, even if the downstream service is slow, and even if you are sending 10 000 requests at same time.

[jjxtra]

Any update on this? Might start on it myself if you haven't already...

[NicolasDorier]

I did not yet @jjxtra if you do I can review.

[jjxtra]

I've started on this, will commit soon

[jjxtra]

Progress and review can be done in async branch (pushed). I had carpal tunnel surgery last week so I am stopping for today :)

[NicolasDorier]

Nice, commented on https://github.com/jjxtra/ExchangeSharp/commit/8137391597c7d6df65267e8c5daaaeee46f8626d#r28578994

[jjxtra]

All commits are in, ready for another set of eyes

[NicolasDorier]

https://github.com/jjxtra/ExchangeSharp/compare/async#diff-0320aa165b129bc681396a8da8831dedR36 is probably not what you want, fat finger bug. (no op)

[NicolasDorier]

I am seeing lot's of missing .ConfigureAwait. That said, it can be done later. I would advise you to put the commit removing the this. in another PR so it is easier to review.

Except this and the bug api = api it seems perfect. Awesome library.

[jjxtra]

Does .ConfigureAwait need to be done anywhere this is an await call?

-- Jeff

On Mon, Apr 16, 2018 at 8:30 PM, Nicolas Dorier notifications@github.com wrote:

I am seeing lot's of missing .ConfigureAwait. That said, it can be done later. I would advise you to put the commit removing the this. in another PR so it is easier to review.

Except this it seems perfect. Awesome library.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93#issuecomment-381811483, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNoGL-bmhimsTYQ-hkfA0y_1WyiE7RTks5tpVPMgaJpZM4TMs9P .

[jjxtra]

Fixed this. using anyalyzer

[NicolasDorier]

@jjxtra it should be done on the first call which can be reached by a UI thread. (so basically the first await of any public method is good canditate).

I prefer using it everywhere to prevent foot shooting though. (opinions might differ)

[jjxtra]

I made all public async methods handle the synchronization context using a helper class. Most public methods on ExchangeAPI are simply sync wrappers to a protected implementation now. That way only ExchangeAPI.cs needs to worry about the sync context. Going forward, I think this is a good pattern to follow.

Thoughts?

[NicolasDorier]

I don't think your method works.

await new SynchronizationContextRemover(); will still want to call the rest of the method on the SynchronizationContext before the call to this method.

I am not 100% sure though. You should try to make a custom Synch Context and check if anything is posted on it.

[jjxtra]

Are you able to perform this test? I have to move on to other projects the rest of this week.

[NicolasDorier]

will try.

[jjxtra]

Got the idea here: https://blogs.msdn.microsoft.com/benwilli/2017/02/09/an-alternative-to-configureawaitfalse-everywhere/

[NicolasDorier]

wow wonderful. I tried it and it works as adverized.

[NicolasDorier]

With this

 class Sync : SynchronizationContext
        {
            public override void Post(SendOrPostCallback d, object state)
            {
                throw new Exception();
                base.Post(d, state);
            }
        }

This cause the Exception of the SynchronizationContext (because Test continuation goes on Sync)

        [Fact]
        public async Task TestSync()
        {
            SynchronizationContext.SetSynchronizationContext(new Sync());
            await Test();
            await Task.Delay(1000);
        }

        private static async Task Test()
        {
            await new SynchronizationContextRemover();
            await Task.Delay(1000);
        }

This does not

        [Fact]
        public async Task TestSync()
        {
            SynchronizationContext.SetSynchronizationContext(new Sync());
            await new SynchronizationContextRemover();
            await Task.Delay(1000);
            await Task.Delay(1000);
        }

So things are working as expected, I love this, should be included into the main framework!

[jjxtra]

Something in the Poloniex unit tests is failing, do you have bandwidth to investigate? The dependancy injection is not working for the request maker. Haven't dug any further then that.

[jjxtra]

Never mind, the tests just need to switch to Request async, will do now and merge.

[NicolasDorier]

wait why would the tests not breaking before but break now?

[jjxtra]

NSubstitute not overriding makerequest

-- Jeff

On Wed, Apr 18, 2018 at 8:17 AM, Nicolas Dorier notifications@github.com wrote:

wait why would the tests not breaking before but break now?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93#issuecomment-382402331, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNoGLchT_pOGYiPSO89Rw4zsQmmBaBuks5tp0rigaJpZM4TMs9P .

[jjxtra]

This is merged in now to master!

[NicolasDorier]

Freaking good! Will use this in BTCPay this week. I love Bitcoin Average but I don't want all merchants complaining to me if they go down :D

Will use your API, then bitcoin average as fallback.

[NicolasDorier]

You made a new nuget package?

[jjxtra]

Replacing my cpu cooler. Pump died after 4 months :( Will make a package tomorrow!

-- Jeff

On Thu, Apr 19, 2018 at 9:15 PM, Nicolas Dorier notifications@github.com wrote:

You made a new nuget package?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/jjxtra/ExchangeSharp/issues/93#issuecomment-382958609, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNoGA1KjDObbQ8cIAAQjCBHjqNPYbnjks5tqVLPgaJpZM4TMs9P .

[NicolasDorier]

any luck? can't wait playing with your lib :D

[jjxtra]

It's out there, may take a few minutes to show up. Version 0.4.0.0