Async not used correctly about exchangesharp HOT 33 CLOSED

from exchangesharp.

from exchangesharp.

from exchangesharp.

@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.

from exchangesharp.

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

from exchangesharp.

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.

from exchangesharp.

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

from exchangesharp.

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

from exchangesharp.

I've started on this, will commit soon

from exchangesharp.

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

from exchangesharp.

Nice, commented on 8137391#r28578994

from exchangesharp.

All commits are in, ready for another set of eyes

from exchangesharp.

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

from exchangesharp.

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.

from exchangesharp.

from exchangesharp.

Fixed this. using anyalyzer

from exchangesharp.

@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)

from exchangesharp.

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?

from exchangesharp.

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.

from exchangesharp.

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

from exchangesharp.

will try.

from exchangesharp.

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

from exchangesharp.

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

from exchangesharp.

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!

from exchangesharp.

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.

from exchangesharp.

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

from exchangesharp.

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

from exchangesharp.

from exchangesharp.

This is merged in now to master!

from exchangesharp.

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.

from exchangesharp.

You made a new nuget package?

from exchangesharp.

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

from exchangesharp.

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

from exchangesharp.