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 @@
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 @@
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 @@
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 @@
this.cacheTelemetry.FinalCacheSize = this.cachedResponses.Count;
this.cacheTelemetry.Dispose();
this.cachedResponses.Dispose();
this.semaphore.Dispose();

Check warning on line 291 in src/Microsoft.ComponentDetection.Detectors/pip/IPyPiClient.cs

View check run for this annotation

Codecov / codecov/patch

src/Microsoft.ComponentDetection.Detectors/pip/IPyPiClient.cs#L291

Added line #L291 was not covered by tests
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
// 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 @@
{
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 @@
/// <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 @@
this.cacheTelemetry.Dispose();
this.cachedProjectWheelFiles.Dispose();
this.cachedSimplePyPiProjects.Dispose();
HttpClient.Dispose();
this.semaphore.Dispose();

Check warning on line 290 in src/Microsoft.ComponentDetection.Detectors/pip/SimplePypiClient.cs

View check run for this annotation

Codecov / codecov/patch

src/Microsoft.ComponentDetection.Detectors/pip/SimplePypiClient.cs#L290

Added line #L290 was not covered by tests
}
}