microdot icon indicating copy to clipboard operation
microdot copied to clipboard

ServiceProxyProvider

Open dinavinter opened this issue 4 years ago • 1 comments

looking at our stats looks like the network time is unjustified long, for example, a random query of one minute:

target.service target.method MAX(stats.network.time)
UserIdService TryGetUserIds 665.7730102539062
PolicyService GetEffectiveSitePolicy 132.62899780273438
SitesService GetSite 230.61199951171875

other stats of the service doesn't indicate any issues so I was searching for some time-consuming logic between RequestStartTimestamp marking and the actually httpClient.PostAsync and I have some concerns about this piece of code: https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L177-L223

  • in case of ServerCertificateVerification or ClientCertificateVerification configured, this lock contains inside the certificate loading which can take time, and we are syncly blocking any attempts to call the service which probably the intention, but what happens to the thread during this lock? https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/HttpsAuthenticator.cs#L35 https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.SharedLogic/Security/WindowsStoreCertificateLocator.cs#L79-L80

  • is it possible that async cache refresh is "syncly" waiting for this lock?https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/Caching/AsyncCache.cs#L300-L314

  • in case of an unreachable error or any type of 'HTTP request exception', we are repeating the process but with tryHttps=false, I'm not sure I completely understood the flow, this is what I see:

    • creating a client for HTTPS: https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L194

    • after the request is failing, creating a client for HTTP: https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L462 https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L211-L214

    • in the next call, we are trying HTTPS again and creating a new client: https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L374

    • if the time fits we are creating two more clients for the reachability checks https://github.com/gigya/microdot/blob/9bdf40cc9b9b224ce83afd1f5ca904a2ddee6ab2/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs#L194-L200

    • after the request is failing, creating a new client for HTTP, and so on and so on


    • what am I missing here?
    • are we performing every HTTP request twice?
    • are we creating two new HTTP clients for every request?
    • are we disposing all of these clients and handlers?
    • this happens every reachability check, plus to calls attempts?
    • what will happen when we are running parallel requests to the same service, even without the certificate, creating an HTTP client and handler is not the cheapest code, and do it each request and within a lock statement sounds expensive.

** see creating multiple HTTP clients effect

dinavinter avatar Dec 16 '20 23:12 dinavinter

@daniel-lamberger @talweiss1982 @aviviadi what do you think?

dinavinter avatar Dec 17 '20 08:12 dinavinter