commit 854750e29736d96c314630a2b259a1ec19aa4339
parent 5f2d19ed4a8e8dd0b895a051f279fe658c97cb27
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Wed, 9 Jun 2021 09:15:44 -0700
Fall back to WinINet if DO download fails (#1138)
There have been several issues caused by DO being disabled by policy, or failing to redirect properly. Due to this, and to prevent other issues from impacting users, we will fall back to the WinINet download code path when DO fails.
Some error codes are intentionally excluded from falling back, largely those pertaining to metered networks. We will add more cases as we find them.
Diffstat:
5 files changed, 69 insertions(+), 10 deletions(-)
diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
@@ -530,6 +530,19 @@ namespace AppInstaller::Logging
}
}
+ void TelemetryTraceLogger::LogNonFatalDOError(std::string_view url, HRESULT hr) const noexcept
+ {
+ if (IsTelemetryEnabled())
+ {
+ AICLI_TraceLoggingWriteActivity(
+ "NonFatalDOError",
+ TraceLoggingUInt32(s_subExecutionId, "SubExecutionId"),
+ AICLI_TraceLoggingStringView(url, "Url"),
+ TraceLoggingHResult(hr, "HResult"),
+ TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance));
+ }
+ }
+
bool TelemetryTraceLogger::IsTelemetryEnabled() const noexcept
{
return g_IsTelemetryProviderEnabled && m_isSettingEnabled && m_isRuntimeEnabled;
diff --git a/src/AppInstallerCommonCore/DODownloader.cpp b/src/AppInstallerCommonCore/DODownloader.cpp
@@ -14,7 +14,13 @@ namespace AppInstaller::Utility
{
namespace DeliveryOptimization
{
-#define DO_E_DOWNLOAD_NO_PROGRESS HRESULT(0x80D02002L) // Download of a file saw no progress within the defined period
+// TODO: Once the SDK headers are available, remove these defines
+#define DO_E_DOWNLOAD_NO_PROGRESS HRESULT(0x80D02002L) // Download of a file saw no progress within the defined period
+
+#define DO_E_BLOCKED_BY_COST_TRANSFER_POLICY HRESULT(0x80D03801L) // DO core paused the job due to cost policy restrictions
+#define DO_E_BLOCKED_BY_CELLULAR_POLICY HRESULT(0x80D03803L) // DO core paused the job due to detection of cellular network and policy restrictions
+#define DO_E_BLOCKED_BY_POWER_STATE HRESULT(0x80D03804L) // DO core paused the job due to detection of power state change into non-AC mode
+#define DO_E_BLOCKED_BY_NO_NETWORK HRESULT(0x80D03805L) // DO core paused the job due to loss of network connectivity
// Represents a download work item for Delivery Optimization.
struct Download
@@ -404,4 +410,15 @@ namespace AppInstaller::Utility
return {};
}
+
+ bool IsDOErrorFatal(HRESULT error)
+ {
+ // If this gets to be large, store in a sorted array and binary search on it.
+ // There will be more to update here, which we should be able to discover through telemetry.
+ return
+ error == DO_E_BLOCKED_BY_COST_TRANSFER_POLICY ||
+ error == DO_E_BLOCKED_BY_CELLULAR_POLICY ||
+ error == DO_E_BLOCKED_BY_POWER_STATE ||
+ error == DO_E_BLOCKED_BY_NO_NETWORK;
+ }
}
diff --git a/src/AppInstallerCommonCore/DODownloader.h b/src/AppInstallerCommonCore/DODownloader.h
@@ -21,4 +21,8 @@ namespace AppInstaller::Utility
IProgressCallback& progress,
bool computeHash,
std::optional<DownloadInfo> info);
+
+ // Returns true if the error from DODownload should be treated as fatal;
+ // false if we should be able to fall back to other download methods.
+ bool IsDOErrorFatal(HRESULT error);
}
diff --git a/src/AppInstallerCommonCore/Downloader.cpp b/src/AppInstallerCommonCore/Downloader.cpp
@@ -7,6 +7,7 @@
#include "Public/AppInstallerSHA256.h"
#include "Public/AppInstallerStrings.h"
#include "Public/AppInstallerLogging.h"
+#include "Public/AppInstallerTelemetry.h"
#include "Public/winget/UserSettings.h"
#include "DODownloader.h"
@@ -167,18 +168,40 @@ namespace AppInstaller::Utility
if (setting == InstallerDownloader::Default ||
setting == InstallerDownloader::DeliveryOptimization)
{
- auto result = DODownload(url, dest, progress, computeHash, info);
- // Since we cannot pre-apply to the file with DO, post-apply the MotW to the file.
- // Only do so if the file exists, because cancellation will not throw here.
- if (std::filesystem::exists(dest))
+ try
+ {
+ auto result = DODownload(url, dest, progress, computeHash, info);
+ // Since we cannot pre-apply to the file with DO, post-apply the MotW to the file.
+ // Only do so if the file exists, because cancellation will not throw here.
+ if (std::filesystem::exists(dest))
+ {
+ ApplyMotwIfApplicable(dest, URLZONE_INTERNET);
+ }
+ return result;
+ }
+ catch (const wil::ResultException& re)
{
- ApplyMotwIfApplicable(dest, URLZONE_INTERNET);
+ // Fall back to WinINet below unless the specific error is not one that should be ignored.
+ // We need to be careful not to bypass metered networks or other reasons that might
+ // intentionally cause the download to be blocked.
+ HRESULT hr = re.GetErrorCode();
+ if (IsDOErrorFatal(hr))
+ {
+ throw;
+ }
+ else
+ {
+ // Send telemetry so that we can understand the reasons for DO failing
+ Logging::Telemetry().LogNonFatalDOError(url, hr);
+ }
}
- return result;
- // If DO becomes an issue, we may choose to catch exceptions and fall back to WinINet below.
- // We would need to be careful not to bypass metered networks or other reasons that might
- // intentionally cause the download to be blocked.
+ // If we reach this point, we are intending to fall through to WinINet.
+ // Remove any file that may have been placed in the target location.
+ if (std::filesystem::exists(dest))
+ {
+ std::filesystem::remove(dest);
+ }
}
}
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h b/src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h
@@ -120,6 +120,8 @@ namespace AppInstaller::Logging
std::string_view arpPublisher,
std::string_view arpLanguage) const noexcept;
+ void LogNonFatalDOError(std::string_view url, HRESULT hr) const noexcept;
+
protected:
TelemetryTraceLogger();
