ServiceProxyProvider

[dinavinter]

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:

microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

Lines 177 to 223 in 9bdf40c

	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?
  

  microdot/Gigya.Microdot.ServiceProxy/HttpsAuthenticator.cs

  Line 35 in 9bdf40c

  clientCert = CertificateLocator.GetCertificate("Client");

  
  

  microdot/Gigya.Microdot.SharedLogic/Security/WindowsStoreCertificateLocator.cs

  Lines 79 to 80 in 9bdf40c

  	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?

  microdot/Gigya.Microdot.ServiceProxy/Caching/AsyncCache.cs

  Lines 300 to 314 in 9bdf40c

  	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:

  • creating a client for HTTPS:

    microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

    Line 194 in 9bdf40c

    if (Not(forceHttps) && httpKey.UseHttps && Not(LastHttpClientKey?.UseHttps ?? false))

  • after the request is failing, creating a client for HTTP:

    microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

    Line 462 in 9bdf40c

    tryHttps = false;

    
    

    microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

    Lines 211 to 214 in 9bdf40c

    	if (!(LastHttpClientKey?.Equals(httpKey) ?? false))
    	{
    	var messageHandler = _httpMessageHandlerFactory(httpKey);
    	var httpClient = CreateHttpClient(messageHandler, httpKey.Timeout);

  • in the next call, we are trying HTTPS again and creating a new client:
    

    microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

    Line 374 in 9bdf40c

    bool tryHttps = GetConfig().TryHttps ?? discoveryConfig.TryHttps;

  • if the time fits we are creating two more clients for the reachability checks
    

    microdot/Gigya.Microdot.ServiceProxy/ServiceProxyProvider.cs

    Lines 194 to 200 in 9bdf40c

    	if (Not(forceHttps) && httpKey.UseHttps && Not(LastHttpClientKey?.UseHttps ?? false))
    	{
    	var now = DateTime.Now;
    	if (now - _lastHttpsTestTime > _httpsTestInterval)
    	{
    	_lastHttpsTestTime = now;
    	RunHttpsAvailabilityTest(httpKey, hostname, basePort);

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

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