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

Users/oakeredolu/throttlehttpclient #872

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ public sealed class PyPiClient : IPyPiClient, IDisposable
private readonly IEnvironmentVariableService environmentVariableService;
private readonly ILogger<PyPiClient> logger;

// Semaphore to limit the number of concurrent calls to pypi.org
private readonly SemaphoreSlim semaphore;

private bool checkedMaxEntriesVariable;

// retries used so far for calls to pypi.org
Expand All @@ -80,6 +83,7 @@ public PyPiClient(IEnvironmentVariableService environmentVariableService, ILogge
FinalCacheSize = 0,
};
this.logger = logger;
this.semaphore = new SemaphoreSlim(5);
}

public static HttpClient HttpClient { get; internal set; } = new HttpClient(HttpClientHandler);
Expand Down Expand Up @@ -246,11 +250,13 @@ private async Task<HttpResponseMessage> GetAndCachePyPiResponseAsync(Uri uri)
return result;
}

await this.semaphore.WaitAsync();
this.logger.LogInformation("Getting Python data from {Uri}", uri);
using var request = new HttpRequestMessage(HttpMethod.Get, uri);
request.Headers.UserAgent.Add(ProductValue);
request.Headers.UserAgent.Add(CommentValue);
var response = await HttpClient.SendAsync(request);
this.semaphore.Release();

// The `first - wins` response accepted into the cache. This might be different from the input if another caller wins the race.
return await this.cachedResponses.GetOrCreateAsync(uri, cacheEntry =>
Expand Down Expand Up @@ -282,5 +288,6 @@ public void Dispose()
this.cacheTelemetry.FinalCacheSize = this.cachedResponses.Count;
this.cacheTelemetry.Dispose();
this.cachedResponses.Dispose();
this.semaphore.Dispose();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ public sealed class SimplePyPiClient : ISimplePyPiClient, IDisposable
// Keep telemetry on how the cache is being used for future refinements
private readonly SimplePypiCacheTelemetryRecord cacheTelemetry = new SimplePypiCacheTelemetryRecord();

// Semaphore to limit the number of concurrent calls to pypi.org
private readonly SemaphoreSlim semaphore;

/// <summary>
/// A thread safe cache implementation which contains a mapping of URI -> SimpleProject for simplepypi api projects
/// and has a limited number of entries which will expire after the cache fills or a specified interval.
Expand All @@ -62,6 +65,7 @@ public SimplePyPiClient(IEnvironmentVariableService environmentVariableService,
{
this.environmentVariableService = environmentVariableService;
this.logger = logger;
this.semaphore = new SemaphoreSlim(5);
}

public static HttpClient HttpClient { get; internal set; } = new HttpClient(HttpClientHandler);
Expand Down Expand Up @@ -265,12 +269,14 @@ private async Task<HttpResponseMessage> RetryPypiRequestAsync(Uri uri, PipDepend
/// <returns> Returns the httpresponsemessage. </returns>
private async Task<HttpResponseMessage> GetPypiResponseAsync(Uri uri)
{
await this.semaphore.WaitAsync();
Copy link
Contributor

@cobya cobya Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of impact does this have on build times for small / large Python repositories? I see in the description only about 1 case.

this.logger.LogInformation("Getting Python data from {Uri}", uri);
using var request = new HttpRequestMessage(HttpMethod.Get, uri);
request.Headers.UserAgent.Add(ProductValue);
request.Headers.UserAgent.Add(CommentValue);
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/vnd.pypi.simple.v1+json"));
var response = await HttpClient.SendAsync(request);
this.semaphore.Release();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do this within a try-catch-finally block which throws the exception again but releases the semaphore in the finally block so exceptions don't block other threads forever.

return response;
}

Expand All @@ -281,6 +287,6 @@ public void Dispose()
this.cacheTelemetry.Dispose();
this.cachedProjectWheelFiles.Dispose();
this.cachedSimplePyPiProjects.Dispose();
HttpClient.Dispose();
this.semaphore.Dispose();
}
}