Async support about tmdblib HOT 25 CLOSED

I'm not personally great at providing Async stuff, but I guess we would simply have to relay the call to the Async versions in Restsharp... (which I strongly assume has Async methods)... Would it be enough to return Task<T> or should we utilize the async keyword?

Luckily most of the library should be simple to port to either form.

from tmdblib.

Apparently Restsharp provides GetAsync and GetAsync as expected.

I can mayb have a look soon to implement it. I will keep you updated.

from tmdblib.

Either that or I dedicate a few hours later today ;).

I wonder if we could do the sync-methods as:
DoAsync().Result

I'm unsure f.ex. How exceptions propagate....

-----Original Message-----
From: "Florian Zysset" [email protected]
Sent: ‎26-‎04-‎2015 14:21
To: "LordMike/TMDbLib" [email protected]
Cc: "Michael Bisbjerg" [email protected]
Subject: Re: [TMDbLib] Async support (#63)

Apparently Restsharp provides GetAsync and GetAsync as expected.
I can mayb have a look soon to implement it. I will keep you updated.
—
Reply to this email directly or view it on GitHub.

from tmdblib.

Task.Result may create deadlocks (see http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html)

I suppose the best way is to use the solution available here : http://stackoverflow.com/questions/5095183/how-would-i-run-an-async-taskt-method-synchronously

But that means exception as UserSessionException that you have, are in the inner exception. But you can extract them to return to the user, I suppose.

from tmdblib.

@scboffspring, see the async branch I just created - commit 40da74d

from tmdblib.

I adjusted the TMDbClientTvSeasons.cs client, as well as the ClientTvSeasonTests.cs file - so look at how it seems and see if that would do.

I imagine that all methods would be Async, and then the end-user would call .Result or similar things to make the methods synchronous.

One caveat though - this requires .Net 4.5

from tmdblib.

@Naliath could also comment on this. If we do this, there'd probably need to be a new 1.0 release to cover the breaking changes.

from tmdblib.

hey guys, yea async would be fine just make sure to mark every await call with .ConfigureAwait(false) else you will get shit with using .Result. From experience issues with async are a pain to fix. should not be a big problem for us. Do remember that when you use ConfigureAwait(false) from that point forward you do no have access to the current context (since it will continue on a different thread), again should not be a problem for us.

In SF for MS Build next week so no access to my dev machine

from tmdblib.

@Naliath should we name the methods Async and make Sync variants?

I've converted the entire library to use async, and adjusted the tests accordingly. There are some tests failing, but from what I can see they shouldn't have worked in the first place based on the output .. so I need to confirm what's happening ...

Also - at what level should we add the ConfigureAwait call?

@scboffspring can you confirm that this is the correct direction?

from tmdblib.

Hi @LordMike,

I suppose for some users that have the Sync version at the moment, it would be great to keep the same version for them. Basically you have two choices :

1. Have an async client and the existing client side-by-side.
2. Have only one client, with each method having it's async one.

Personnaly I'd opt for option number 2. @Naliath advice on that ?

Regarding the ConfigureAwait, I suppose it is required only on the entry method, as anyway the context captured will not be the calling context for sub-methods. But @Naliath looks more able to answer this question :)

(BTW interesting article I found a few month ago about async/await and context : http://blog.stephencleary.com/2012/02/async-and-await.html)

from tmdblib.

As for where to place the ConfigureAwait the answer is easy, everywhere. Else you will end up with one call that doesn't have it causing some mysterious bug that has you looking for days where the problem is comming from. Besides it does no harm as long as you don't need the calling thread context (it's also the offical recommendation).

quote from the referenced article also confirms this "A good rule of thumb is to use ConfigureAwait(false) unless you know you do need the context."

If we want to follow the recomendations we should name our methods with the "Async" suffix. Since it is possible to use the ".Result" helper to get a sync result I would stick with just one client. This will be easier to maintain for us as well.

The Async/Await principle is here to stay so I say we just focus on that.

from tmdblib.

@Naliath see the latest commit. That should cover the ConfigureAwait requirement.

from tmdblib.

late reply, but yea looks good 👍

from tmdblib.

Just updated the async branch with the latest commits from master. I should probably push this to a Nuget Package so it can be tested.

from tmdblib.

I'm having trouble implementing the retry logic and the rate limit handling for the async method. We currently overload a RestSharp client method (Execute), which is not called on async methods. Instead ExecuteTaskAsync is called - and it has a different design.

If someone could give me pointers, or implement that part, it'd be awesome.

Stub is here: 6f18f5c

from tmdblib.

just did a checkin, not sure where it ened up. It's like my branch but still linked to yours? Not a regular git user so not sure what exactly I did there.

from tmdblib.

It ended here: https://github.com/LordMike/TMDbLib/commits/async
So you did that part correct :).

from tmdblib.

odd that I couldn't just connect to your branch and had to create a new one. When I tried to remove mine it removed yours so I just restored it.

from tmdblib.

Oddity. Anyhows, at least the RateLimit test works now.
b785998

from tmdblib.

yea task based programming throws those when ever you are not using the regular await syntax. It's a real bitch if you ask me. It's either you go all async or not at all for me

from tmdblib.

@Naliath your retry method seems to work really well. There are only very few tests that fail (some I fixed).. I'll probably look a little more into it.

from tmdblib.

@scboffspring, have you had a chance to try out the code?

I'd love to make a 0.9 or 1.0 release :P

from tmdblib.

Hi @LordMike

Sadly not, I had to put my project temporarly on hold because of job. I hope to continue next week on it.

I will keep you updated as soon as I have done some testing

from tmdblib.

I just brushed up the async branch to incorporate the latest changes from all the other PR's so far.

from tmdblib.

Merged into master. Lets move it :)

from tmdblib.