Skip to content

Commit 5a83707

Browse files
JusterZhuclaude
andcommitted
fix: address Copilot review comments on PR #456/#458
- Remove empty try/finally block in ClientUpdateStrategy download path - Add thread-safety note for shared custom executor in DefaultDownloadOrchestrator - Add ResolveExtensionType<T>() to avoid wasteful instantiation for pipeline factory - Restore ClientUpdateStrategy(IDownloadOrchestrator?) constructor for backward compat Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 20782a3 commit 5a83707

4 files changed

Lines changed: 19 additions & 11 deletions

File tree

src/c#/GeneralUpdate.Core/Bootstrap/GeneralUpdateBootstrap.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,11 @@ private async Task<GeneralUpdateBootstrap> LaunchWithStrategy(IStrategy roleStra
109109
var downloadExecutor = ResolveExtension<Download.Abstractions.IDownloadExecutor>();
110110

111111
// Build download pipeline factory from registered extension type
112+
// Uses ResolveExtensionType to avoid wasteful instantiation just to get the Type
112113
Func<string?, Download.Abstractions.IDownloadPipeline>? downloadPipelineFactory = null;
113-
var downloadPipeline = ResolveExtension<Download.Abstractions.IDownloadPipeline>();
114-
if (downloadPipeline != null)
114+
var pipelineType = ResolveExtensionType<Download.Abstractions.IDownloadPipeline>();
115+
if (pipelineType != null)
115116
{
116-
var pipelineType = downloadPipeline.GetType();
117117
var stringCtor = pipelineType.GetConstructor([typeof(string)]);
118118
if (stringCtor != null)
119119
downloadPipelineFactory = hash => (Download.Abstractions.IDownloadPipeline)stringCtor.Invoke([hash]);

src/c#/GeneralUpdate.Core/Configuration/AbstractBootstrap.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,5 +109,11 @@ protected T GetOption<T>(UpdateOption<T>? option)
109109
return instance as TExtension;
110110
return null;
111111
}
112+
113+
/// <summary>Resolves the registered extension type without instantiating it.</summary>
114+
protected Type? ResolveExtensionType<TExtension>() where TExtension : class
115+
{
116+
return _extensions.TryGetValue(typeof(TExtension), out var t) ? t : null;
117+
}
112118
}
113119
}

src/c#/GeneralUpdate.Core/Download/Orchestrators/DefaultDownloadOrchestrator.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ public class DefaultDownloadOrchestrator : IDownloadOrchestrator
3030
private readonly DownloadOrchestratorOptions _options;
3131
private readonly IDownloadExecutor? _customExecutor;
3232
private readonly Func<string?, IDownloadPipeline>? _pipelineFactory;
33+
// Note: _customExecutor is a single shared instance used across parallel asset downloads.
34+
// Implementations of IDownloadExecutor must be thread-safe. HttpClient-based executors
35+
// satisfy this as HttpClient is designed for concurrent use.
3336

3437
public DefaultDownloadOrchestrator(
3538
HttpClient httpClient,

src/c#/GeneralUpdate.Core/Strategy/ClientUpdateStrategy.cs

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ private enum UpdateScenario
6060

6161
public ClientUpdateStrategy() { }
6262

63+
public ClientUpdateStrategy(Download.Abstractions.IDownloadOrchestrator? orchestrator)
64+
=> _orchestrator = orchestrator;
65+
6366
/// <summary>Sets a custom OS-level strategy. When set, replaces automatic platform detection.</summary>
6467
public void SetOsStrategy(IStrategy? strategy) => _customOsStrategy = strategy;
6568

@@ -264,14 +267,10 @@ private async Task ExecuteStandardWorkflowAsync()
264267
else
265268
{
266269
var httpClient = GeneralUpdate.Core.Network.HttpClientProvider.Shared;
267-
try
268-
{
269-
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(
270-
httpClient, orchOptions, _customDownloadPolicy,
271-
_customDownloadExecutor, _customDownloadPipelineFactory);
272-
await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
273-
}
274-
finally { }
270+
var orchestrator = new Download.Orchestrators.DefaultDownloadOrchestrator(
271+
httpClient, orchOptions, _customDownloadPolicy,
272+
_customDownloadExecutor, _customDownloadPipelineFactory);
273+
await orchestrator.ExecuteAsync(downloadPlan, _configInfo.TempPath).ConfigureAwait(false);
275274
}
276275

277276
await SafeReportDownloadCompletedAsync(hooksCtx).ConfigureAwait(false);

0 commit comments

Comments
 (0)