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

Honor 429 Retry-After #3718

Merged
merged 18 commits into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from 11 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
2 changes: 2 additions & 0 deletions .github/actions/spelling/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ LOWORD
LPARAM
LPBYTE
LPCWSTR
lpdw
LPDWORD
lpfn
LPGRPICONDIR
Expand Down Expand Up @@ -483,6 +484,7 @@ ucasemap
UChars
ucnv
uec
UNAVAIL
uninitialize
unins
uninstallation
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(TooManyArgError);
WINGET_DEFINE_RESOURCE_STRINGID(TooManyBehaviorsError);
WINGET_DEFINE_RESOURCE_STRINGID(UnableToPurgeInstallDirectory);
WINGET_DEFINE_RESOURCE_STRINGID(Unavailable);
WINGET_DEFINE_RESOURCE_STRINGID(UnexpectedErrorExecutingCommand);
WINGET_DEFINE_RESOURCE_STRINGID(UninstallAbandoned);
WINGET_DEFINE_RESOURCE_STRINGID(UninstallCommandLongDescription);
Expand Down
12 changes: 10 additions & 2 deletions src/AppInstallerCLICore/Workflows/SourceFlow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,17 @@ namespace AppInstaller::CLI::Workflow
Repository::Source source{ sd.Name };
context.Reporter.Info() << Resource::String::SourceUpdateOne(Utility::LocIndView{ sd.Name }) << std::endl;
auto updateFunction = [&](IProgressCallback& progress)->std::vector<Repository::SourceDetails> { return source.Update(progress); };
if (!context.Reporter.ExecuteWithProgress(updateFunction).empty())
auto sourceDetails = context.Reporter.ExecuteWithProgress(updateFunction);
if (!sourceDetails.empty())
{
context.Reporter.Info() << Resource::String::Cancelled << std::endl;
if (std::chrono::system_clock::now() < sourceDetails[0].DoNotUpdateBefore)
{
context.Reporter.Warn() << Resource::String::Unavailable << std::endl;
}
else
{
context.Reporter.Info() << Resource::String::Cancelled << std::endl;
}
}
else
{
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
Original file line number Diff line number Diff line change
Expand Up @@ -2581,4 +2581,7 @@ Please specify one of them using the --source option to proceed.</value>
<data name="WINGET_CONFIG_ERROR_UNIT_SETTING_CONFIG_ROOT" xml:space="preserve">
<value>A unit contains a setting that requires the config root.</value>
</data>
<data name="Unavailable" xml:space="preserve">
<value>Unavailable</value>
</data>
</root>
145 changes: 136 additions & 9 deletions src/AppInstallerCommonCore/Downloader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,80 @@
using namespace AppInstaller::Runtime;
using namespace AppInstaller::Settings;
using namespace AppInstaller::Filesystem;
using namespace AppInstaller::Utility::HttpStream;
using namespace winrt::Windows::Web::Http;
using namespace winrt::Windows::Web::Http::Headers;
using namespace winrt::Windows::Web::Http::Filters;

namespace AppInstaller::Utility
{
namespace
{
// Gets the retry after value in terms of a delay in seconds
std::chrono::seconds GetRetryAfter(const HttpDateOrDeltaHeaderValue& retryAfter)
{
if (retryAfter)
{
auto delta = retryAfter.Delta();
if (delta)
{
return std::chrono::duration_cast<std::chrono::seconds>(delta.GetTimeSpan());
}

auto dateTimeRef = retryAfter.Date();
if (dateTimeRef)
{
auto dateTime = dateTimeRef.GetDateTime();
auto now = winrt::clock::now();

if (dateTime > now)
{
return std::chrono::duration_cast<std::chrono::seconds>(dateTime - now);
}
}
}

return 0s;
}

std::chrono::seconds GetRetryAfter(const wil::unique_hinternet& urlFile)
{
std::wstring retryAfter = {};
DWORD length = 0;
if (!HttpQueryInfo(urlFile.get(),
msftrubengu marked this conversation as resolved.
Show resolved Hide resolved
HTTP_QUERY_RETRY_AFTER,
&retryAfter,
&length,
nullptr))
{
auto lastError = GetLastError();
if (lastError == ERROR_INSUFFICIENT_BUFFER)
{
// lpdwBufferLength contains the size, in bytes, of a buffer large enough to receive the requested information
// without the nul char. not the exact buffer size.
msftrubengu marked this conversation as resolved.
Show resolved Hide resolved
auto size = static_cast<size_t>(length) / sizeof(wchar_t);
retryAfter.resize(size + 1);
if (HttpQueryInfo(urlFile.get(),
HTTP_QUERY_RETRY_AFTER,
&retryAfter[0],
&length,
nullptr))
{
// because the buffer can be bigger remove possible null chars
retryAfter.erase(retryAfter.find(L'\0'));
return AppInstaller::Utility::GetRetryAfter(retryAfter);
}
}
else
{
AICLI_LOG(Core, Error, << "Error retrieving Retry-After header: " << GetLastError());
}
}

return 0s;
}
}

std::optional<std::vector<BYTE>> WinINetDownloadToStream(
const std::string& url,
std::ostream& dest,
Expand Down Expand Up @@ -63,8 +131,19 @@ namespace AppInstaller::Utility
&cbRequestStatus,
nullptr), "Query download request status failed.");

if (requestStatus != HTTP_STATUS_OK)
constexpr DWORD TooManyRequest = 429;

switch (requestStatus)
{
case HTTP_STATUS_OK:
// All good
break;
case TooManyRequest:
case HTTP_STATUS_SERVICE_UNAVAIL:
{
THROW_EXCEPTION(ServiceUnavailableException(GetRetryAfter(urlFile)));
}
default:
AICLI_LOG(Core, Error, << "Download request failed. Returned status: " << requestStatus);
THROW_HR_MSG(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, requestStatus), "Download request status is not success.");
}
Expand Down Expand Up @@ -160,9 +239,19 @@ namespace AppInstaller::Utility

HttpResponseMessage response = client.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead).get();

THROW_HR_IF(
MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()),
response.StatusCode() != HttpStatusCode::Ok);
switch (response.StatusCode())
{
case HttpStatusCode::Ok:
// All good
break;
case HttpStatusCode::TooManyRequests:
case HttpStatusCode::ServiceUnavailable:
{
THROW_EXCEPTION(ServiceUnavailableException(GetRetryAfter(response.Headers().RetryAfter())));
}
default:
THROW_HR(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()));
}

std::map<std::string, std::string> result;

Expand Down Expand Up @@ -409,12 +498,28 @@ namespace AppInstaller::Utility
{
// Get an IStream from the input uri and try to create package or bundler reader.
winrt::Windows::Foundation::Uri uri(Utility::ConvertToUTF16(uriStr));
auto randomAccessStream = HttpStream::HttpRandomAccessStream::CreateAsync(uri).get();

::IUnknown* rasAsIUnknown = (::IUnknown*)winrt::get_abi(randomAccessStream);
THROW_IF_FAILED(CreateStreamOverRandomAccessStream(
rasAsIUnknown,
IID_PPV_ARGS(inputStream.ReleaseAndGetAddressOf())));
winrt::com_ptr<HttpRandomAccessStream> httpRandomAccessStream = winrt::make_self<HttpRandomAccessStream>();

try
{
auto randomAccessStream = httpRandomAccessStream->InitializeAsync(uri).get();

::IUnknown* rasAsIUnknown = (::IUnknown*)winrt::get_abi(randomAccessStream);
THROW_IF_FAILED(CreateStreamOverRandomAccessStream(
rasAsIUnknown,
IID_PPV_ARGS(inputStream.ReleaseAndGetAddressOf())));
}
catch (const winrt::hresult_error& hre)
{
if (hre.code() == APPINSTALLER_CLI_ERROR_SERVICE_UNAVAILABLE)
{
auto retryAfter = httpRandomAccessStream->RetryAfter();
THROW_EXCEPTION(AppInstaller::Utility::ServiceUnavailableException(retryAfter));
Copy link
Member

Choose a reason for hiding this comment

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

Did you verify this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

}

throw;
}
}
else
{
Expand All @@ -425,4 +530,26 @@ namespace AppInstaller::Utility

return inputStream;
}

std::chrono::seconds GetRetryAfter(const std::wstring& retryAfter)
{
try
{
winrt::hstring hstringValue{ retryAfter };
HttpDateOrDeltaHeaderValue headerValue = nullptr;
HttpDateOrDeltaHeaderValue::TryParse(hstringValue, headerValue);
return GetRetryAfter(headerValue);
}
catch (...)
{
AICLI_LOG(Core, Error, << "Retry-After value not supported: " << Utility::ConvertToUTF8(retryAfter));
}

return 0s;
}

std::chrono::seconds GetRetryAfter(const HttpResponseMessage& response)
{
return GetRetryAfter(response.Headers().RetryAfter());
}
}
32 changes: 29 additions & 3 deletions src/AppInstallerCommonCore/HttpStream/HttpClientWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "Public/AppInstallerStrings.h"
#include "HttpClientWrapper.h"
#include "Public/AppInstallerRuntime.h"
#include "Public/AppInstallerDownloader.h"

using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Security::Cryptography;
Expand Down Expand Up @@ -47,9 +48,19 @@ namespace AppInstaller::Utility::HttpStream

HttpResponseMessage response = co_await m_httpClient.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead);

THROW_HR_IF(
MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()),
response.StatusCode() != HttpStatusCode::Ok);
switch (response.StatusCode())
{
case HttpStatusCode::Ok:
// All good
break;
case HttpStatusCode::TooManyRequests:
case HttpStatusCode::ServiceUnavailable:
{
THROW_EXCEPTION(ServiceUnavailableException(GetRetryAfter(response)));
}
default:
THROW_HR(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()));
}

// Get the length from the response
if (response.Content().Headers().HasKey(L"Content-Length"))
Expand Down Expand Up @@ -106,6 +117,21 @@ namespace AppInstaller::Utility::HttpStream
HttpResponseMessage response = co_await m_httpClient.SendRequestAsync(request, HttpCompletionOption::ResponseHeadersRead);
HttpContentHeaderCollection contentHeaders = response.Content().Headers();

switch (response.StatusCode())
{
case HttpStatusCode::Ok:
case HttpStatusCode::PartialContent:
// All good
break;
case HttpStatusCode::TooManyRequests:
case HttpStatusCode::ServiceUnavailable:
{
THROW_EXCEPTION(ServiceUnavailableException(GetRetryAfter(response)));
}
default:
THROW_HR(MAKE_HRESULT(SEVERITY_ERROR, FACILITY_HTTP, response.StatusCode()));
}

if (response.StatusCode() != HttpStatusCode::PartialContent && startPosition != 0)
{
// throw HRESULT used for range-request error
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#include "pch.h"
#include "HttpRandomAccessStream.h"
#include "Public/AppInstallerDownloader.h"

using namespace winrt::Windows::Foundation;
using namespace winrt::Windows::Storage::Streams;
Expand All @@ -12,16 +13,23 @@ using namespace winrt::Windows::Storage::Streams;
// The HRESULTs will be mapped to UI error code by the appropriate component
namespace AppInstaller::Utility::HttpStream
{
IAsyncOperation<IRandomAccessStream> HttpRandomAccessStream::CreateAsync(const Uri& uri)
IAsyncOperation<IRandomAccessStream> HttpRandomAccessStream::InitializeAsync(const Uri& uri)
{
winrt::com_ptr<HttpRandomAccessStream> stream = winrt::make_self<HttpRandomAccessStream>();

stream->m_httpHelper = co_await HttpClientWrapper::CreateAsync(uri);
stream->m_size = stream->m_httpHelper->GetFullFileSize();
stream->m_httpLocalCache = std::make_unique<HttpLocalCache>();

co_return stream.as<IRandomAccessStream>();

auto strong_this{ get_strong() };

try
{
strong_this->m_httpHelper = co_await HttpClientWrapper::CreateAsync(uri);
strong_this->m_size = strong_this->m_httpHelper->GetFullFileSize();
strong_this->m_httpLocalCache = std::make_unique<HttpLocalCache>();
}
catch (const ServiceUnavailableException& e)
{
strong_this->m_retryAfter = e.RetryAfter();
throw;
}

co_return strong_this.as<IRandomAccessStream>();
}

uint64_t HttpRandomAccessStream::Size() const
Expand Down Expand Up @@ -86,4 +94,9 @@ namespace AppInstaller::Utility::HttpStream

co_return result;
}

std::chrono::seconds HttpRandomAccessStream::RetryAfter() const
{
return m_retryAfter;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include "HttpClientWrapper.h"
#include "HttpLocalCache.h"

using namespace std::chrono_literals;

namespace AppInstaller::Utility::HttpStream
{
// Provides an implementation of a random access stream over HTTP that supports
Expand All @@ -17,7 +19,7 @@ namespace AppInstaller::Utility::HttpStream
winrt::Windows::Storage::Streams::IInputStream>
{
public:
static winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::Storage::Streams::IRandomAccessStream> CreateAsync(
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::Storage::Streams::IRandomAccessStream> InitializeAsync(
const winrt::Windows::Foundation::Uri& uri);
uint64_t Size() const;
void Size(uint64_t value);
Expand All @@ -32,11 +34,13 @@ namespace AppInstaller::Utility::HttpStream
winrt::Windows::Storage::Streams::IBuffer buffer,
uint32_t count,
winrt::Windows::Storage::Streams::InputStreamOptions options);
std::chrono::seconds RetryAfter() const;

private:
std::shared_ptr<HttpClientWrapper> m_httpHelper;
std::unique_ptr<HttpLocalCache> m_httpLocalCache;
unsigned long long m_size = 0;
unsigned long long m_requestedPosition = 0;
std::chrono::seconds m_retryAfter = 0s;
};
}
2 changes: 2 additions & 0 deletions src/AppInstallerCommonCore/MsixInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,8 @@ namespace AppInstaller::Msix
MsixInfo::MsixInfo(std::string_view uriStr)
{
m_stream = Utility::GetReadOnlyStreamFromURI(uriStr);

// Here if thrown APPINSTALLER_CLI_ERROR_SERVICE_UNAVAILABLE somehow get RetryAfter
msftrubengu marked this conversation as resolved.
Show resolved Hide resolved
if (GetBundleReader(m_stream.Get(), &m_bundleReader))
{
m_isBundle = true;
Expand Down
Loading
Loading