Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ServiceProxyProvider #349

Open
dinavinter opened this issue Dec 16, 2020 · 1 comment
Open

ServiceProxyProvider #349

dinavinter opened this issue Dec 16, 2020 · 1 comment

Comments

@dinavinter
Copy link

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:

private (HttpClient httpClient, bool isHttps) GetHttpClient(ServiceDiscoveryConfig serviceConfig, DiscoveryConfig defaultConfig, bool tryHttps, string hostname, int basePort)
{
var forceHttps = serviceConfig.UseHttpsOverride ?? (ServiceInterfaceRequiresHttps || defaultConfig.UseHttpsOverride);
var useHttps = tryHttps || forceHttps;
string securityRole = serviceConfig.SecurityRole;
var verificationMode = serviceConfig.ServerCertificateVerification ??
defaultConfig.ServerCertificateVerification;
var supplyClientCertificate = (serviceConfig.ClientCertificateVerification ?? defaultConfig.ClientCertificateVerification)
== ClientCertificateVerificationMode.VerifyIdenticalRootCertificate;
var httpKey = new HttpClientConfiguration(useHttps, securityRole, serviceConfig.RequestTimeout, verificationMode, supplyClientCertificate);
lock (HttpClientLock)
{
if (LastHttpClient != null && LastHttpClientKey.Equals(httpKey))
return ( httpClient: LastHttpClient, isHttps: useHttps);
// In case we're trying HTTPs and the previous request on this instance was HTTP (or if this is the first request)
if (Not(forceHttps) && httpKey.UseHttps && Not(LastHttpClientKey?.UseHttps ?? false))
{
var now = DateTime.Now;
if (now - _lastHttpsTestTime > _httpsTestInterval)
{
_lastHttpsTestTime = now;
RunHttpsAvailabilityTest(httpKey, hostname, basePort);
}
httpKey = new HttpClientConfiguration(
useHttps: false,
securityRole: null,
timeout:httpKey.Timeout,
verificationMode:httpKey.VerificationMode,
supplyClientCertificate: httpKey.SupplyClientCertificate);
}
if (!(LastHttpClientKey?.Equals(httpKey) ?? false))
{
var messageHandler = _httpMessageHandlerFactory(httpKey);
var httpClient = CreateHttpClient(messageHandler, httpKey.Timeout);
LastHttpClient = httpClient;
LastHttpClientKey = httpKey;
_httpMessageHandler = messageHandler;
}
return (httpClient: LastHttpClient, isHttps: httpKey.UseHttps);
}
}

  • 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?

    clientCert = CertificateLocator.GetCertificate("Client");

    var store = new X509Store(storeName, storeLocation);
    store.Open(OpenFlags.OpenExistingOnly | OpenFlags.ReadOnly);

  • is it possible that async cache refresh is "syncly" waiting for this lock?

    existingItem.RefreshTask = ((Func<Task>)(async () =>
    {
    try
    {
    var getNewValue = WrappedFactory(false);
    await getNewValue.ConfigureAwait(false);
    existingItem.CurrentValueTask = getNewValue;
    existingItem.NextRefreshTime = DateTime.UtcNow + policy.RefreshTime;
    MemoryCache.Set(new CacheItem(key, existingItem), policy);
    }
    catch
    {
    existingItem.NextRefreshTime = DateTime.UtcNow + policy.FailedRefreshDelay;
    }
    })).Invoke();

  • 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:


    • 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 dinavinter changed the title ServiceProxyProvider Performance Concerns ServiceProxyProvider Dec 16, 2020
@dinavinter
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant