search

ocinbat / Sillycore

MIT License
15 stars 7 forks source link

SillycoryRestClient thread safety #23

Open kutluarasli opened 6 years ago

kutluarasli commented 6 years ago

According to RestSharp (which is underlying dependecy of SillycoreRestClient) discussions, RestSharp.RestClient has thread safety issues.

This also hurt SillycoreRestClient thread safety and creating a new client for each request drives a new access token to be retrieved everytime.

Possible Solutions:

1) Access token lifecycle and RestSharp.RestClient lifecycle may be decoupled which is handled in OAuth2Decorator. SillycoreRestClient can inject CachingTokenClient and Oath2Decorator is initiated for each call under the hood (or can be initiated for each call).

2) Giving up RestSharp (which will break the interface). I just said :)

I expect to hear you comments :)

ocinbat commented 6 years ago

I remember implementing a static _accessToken variable hold inside OAuth2Decorator. If my memory serves me well, we changed it due to having multiple REST api's being consumed in one project, which is completely normal and it was stupid not to think of it at first place, and access tokens for those API's were being ambigious. Now aas far as I can see all we need is to hold a "static" access token collection which makes us enable to remember which token is for which API. That kinda list seems to me as a dictionary. :P So If we change _accessToken in OAuth2Decorator to a static Dictionary<clientId, accessToken> it would probably work. :P

What do you think?

kutluarasli commented 6 years ago

That sounds good enough to me. I will implement the change and send it for your review.

kutluarasli commented 6 years ago

@ocinbat Here is my draft implementation in my own fork. Not tested yet!

I think, that's what you meant:

https://github.com/kutluarasli/Sillycore/commit/02fcf4464577bf750d2c2b095e0032b7e5e12579

Some extra implementation could be:

1) Using ConcurrentDictionary. Trade-off is: Absence of ConcurrentDictionary may lead to some extra missed request . Existance of ConcurrentDictionary may hurt performance.

2) Implementing a TTL for Dictionary and clear expired items. That's not a big issue, I believe.

I didn't like the way I built cache key; but that's the one I find safe in some cases. I am open for suggestions.