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.

Sign up for GitHub

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

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Forwarding the LanguageWorkerOptions from the host to the job host scope and ensuring we use the provided instances. #10369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
af614bb
Forwarding the LanguageWorkerOptions from the host
fabiocav Aug 6, 2024
bfbfe56
Adding a change notification system to refresh Options. Using this to…
kshyju Sep 10, 2024
a1257e7
Fixed a test to reflect the new code changes.
kshyju Sep 11, 2024
eec5806
Shutdown channel when language worker options change
kshyju Sep 18, 2024
706dc1f
Fix test to reflect the exception message change.
kshyju Sep 27, 2024
624a870
Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs
fabiocav Oct 8, 2024
1b8cff0
Update src/WebJobs.Script/ScriptHostBuilderExtensions.cs
fabiocav Oct 8, 2024
219cdf2
Merge branch 'dev' into fabiocav/iomlwo
kshyju Nov 4, 2024
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
28 changes: 28 additions & 0 deletions src/WebJobs.Script.WebHost/Configuration/HostBuiltChangeTokenSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

using System;
using System.Threading;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Azure.WebJobs.Script.WebHost.Configuration
{
public sealed class HostBuiltChangeTokenSource<TOptions> : IOptionsChangeTokenSource<TOptions>, IDisposable
{
private CancellationTokenSource _cts = new();

public string Name => Options.DefaultName;

public IChangeToken GetChangeToken() => new CancellationChangeToken(_cts.Token);

public void TriggerChange()
{
var previousCts = Interlocked.Exchange(ref _cts, new CancellationTokenSource());
previousCts.Cancel();
previousCts.Dispose();
}

public void Dispose() => _cts.Dispose();
}
}
7 changes: 5 additions & 2 deletions src/WebJobs.Script.WebHost/WebHostServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,16 +208,19 @@ public static void AddWebJobsScriptHost(this IServiceCollection services, IConfi
// will reset the ScriptApplicationHostOptions only after StandbyOptions have been reset.
services.ConfigureOptions<ScriptApplicationHostOptionsSetup>();
services.AddSingleton<IOptionsChangeTokenSource<ScriptApplicationHostOptions>, ScriptApplicationHostOptionsChangeTokenSource>();

services.ConfigureOptions<StandbyOptionsSetup>();
services.ConfigureOptionsWithChangeTokenSource<LanguageWorkerOptions, LanguageWorkerOptionsSetup, SpecializationChangeTokenSource<LanguageWorkerOptions>>();
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
services.ConfigureOptionsWithChangeTokenSource<AppServiceOptions, AppServiceOptionsSetup, SpecializationChangeTokenSource<AppServiceOptions>>();
services.ConfigureOptionsWithChangeTokenSource<HttpBodyControlOptions, HttpBodyControlOptionsSetup, SpecializationChangeTokenSource<HttpBodyControlOptions>>();
services.ConfigureOptions<FlexConsumptionMetricsPublisherOptionsSetup>();
services.ConfigureOptions<ConsoleLoggingOptionsSetup>();
services.AddHostingConfigOptions(configuration);
services.ConfigureOptions<ExtensionRequirementOptionsSetup>();

// Refresh LanguageWorkerOptions when HostBuiltChangeTokenSource is triggered.
services.AddSingleton<HostBuiltChangeTokenSource<LanguageWorkerOptions>>();
services.AddSingleton<IOptionsChangeTokenSource<LanguageWorkerOptions>>(s => s.GetRequiredService<HostBuiltChangeTokenSource<LanguageWorkerOptions>>());

services.TryAddSingleton<IDependencyValidator, DependencyValidator>();
services.TryAddSingleton<IJobHostMiddlewarePipeline>(s => DefaultMiddlewarePipeline.Empty);

Expand Down
8 changes: 7 additions & 1 deletion src/WebJobs.Script.WebHost/WebJobsScriptHostService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using Microsoft.Azure.WebJobs.Script.Eventing;
using Microsoft.Azure.WebJobs.Script.Metrics;
using Microsoft.Azure.WebJobs.Script.Scale;
using Microsoft.Azure.WebJobs.Script.WebHost.Configuration;
using Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics;
using Microsoft.Azure.WebJobs.Script.WebHost.Diagnostics.Extensions;
using Microsoft.Azure.WebJobs.Script.Workers;
Expand Down Expand Up @@ -61,6 +62,7 @@ public class WebJobsScriptHostService : IHostedService, IScriptHostManager, ISer
private readonly bool _originalStandbyModeValue;
private readonly string _originalFunctionsWorkerRuntime;
private readonly string _originalFunctionsWorkerRuntimeVersion;
private readonly HostBuiltChangeTokenSource<LanguageWorkerOptions> _hostBuiltChangeTokenSourceForLanguageWorkerOptions;
private IScriptEventManager _eventManager;

private IHost _host;
Expand All @@ -79,7 +81,8 @@ public WebJobsScriptHostService(IOptionsMonitor<ScriptApplicationHostOptions> ap
IScriptWebHostEnvironment scriptWebHostEnvironment, IEnvironment environment,
HostPerformanceManager hostPerformanceManager, IOptions<HostHealthMonitorOptions> healthMonitorOptions,
IMetricsLogger metricsLogger, IApplicationLifetime applicationLifetime, IConfiguration config, IScriptEventManager eventManager, IHostMetrics hostMetrics,
IOptions<FunctionsHostingConfigOptions> hostingConfigOptions)
IOptions<FunctionsHostingConfigOptions> hostingConfigOptions,
HostBuiltChangeTokenSource<LanguageWorkerOptions> hostBuiltChangeTokenSource)
{
ArgumentNullException.ThrowIfNull(loggerFactory);

Expand All @@ -90,6 +93,7 @@ public WebJobsScriptHostService(IOptionsMonitor<ScriptApplicationHostOptions> ap
RegisterApplicationLifetimeEvents();

_metricsLogger = metricsLogger;
_hostBuiltChangeTokenSourceForLanguageWorkerOptions = hostBuiltChangeTokenSource ?? throw new ArgumentNullException(nameof(hostBuiltChangeTokenSource));
_applicationHostOptions = applicationHostOptions ?? throw new ArgumentNullException(nameof(applicationHostOptions));
_scriptWebHostEnvironment = scriptWebHostEnvironment ?? throw new ArgumentNullException(nameof(scriptWebHostEnvironment));
_scriptHostBuilder = scriptHostBuilder ?? throw new ArgumentNullException(nameof(scriptHostBuilder));
Expand Down Expand Up @@ -351,6 +355,8 @@ private async Task UnsynchronizedStartHostAsync(ScriptHostStartupOperation activ

ActiveHost = localHost;

_hostBuiltChangeTokenSourceForLanguageWorkerOptions.TriggerChange();

// Forward initial startup logs to AppInsights/OpenTelemetry.
// These are not tracked by the AppInsights and OpenTelemetry logger provider as these are added in the script host.
var loggerProviders = ActiveHost.Services.GetServices<ILoggerProvider>();
Expand Down
10 changes: 4 additions & 6 deletions src/WebJobs.Script/Description/FunctionDescriptorProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,18 @@ private bool TryParseTriggerParameter(BindingMetadata metadata, out ParameterDes

protected internal virtual void ValidateFunction(FunctionMetadata functionMetadata)
{
HashSet<string> names = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
HashSet<string> names = new(StringComparer.OrdinalIgnoreCase);
foreach (var binding in functionMetadata.Bindings)
{
ValidateBinding(binding);

// Ensure no duplicate binding names
if (names.Contains(binding.Name))
{
throw new InvalidOperationException(string.Format("Multiple bindings with name '{0}' discovered. Binding names must be unique.", binding.Name));
}
else
{
names.Add(binding.Name);
throw new InvalidOperationException($"{nameof(FunctionDescriptorProvider)}: Multiple bindings with name '{binding.Name}' discovered. Binding names must be unique.");
}

names.Add(binding.Name);
}

// Verify there aren't multiple triggers defined
Expand Down
8 changes: 3 additions & 5 deletions src/WebJobs.Script/Host/WorkerFunctionMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,13 +257,11 @@ internal static FunctionMetadata ValidateBindings(IEnumerable<string> rawBinding
// Ensure no duplicate binding names exist
if (bindingNames.Contains(functionBinding.Name))
{
throw new InvalidOperationException(string.Format("Multiple bindings with name '{0}' discovered. Binding names must be unique.", functionBinding.Name));
}
else
{
bindingNames.Add(functionBinding.Name);
throw new InvalidOperationException($"{nameof(WorkerFunctionDescriptorProvider)}: Multiple bindings with name '{functionBinding.Name}' discovered. Binding names must be unique.");
}

bindingNames.Add(functionBinding.Name);

// add binding to function.Bindings once validation is complete
function.Bindings.Add(functionBinding);
}
Expand Down
14 changes: 11 additions & 3 deletions src/WebJobs.Script/ScriptHostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,10 +320,9 @@ public static IHostBuilder AddScriptHostCore(this IHostBuilder builder, ScriptAp
// Configuration
services.AddSingleton<IOptions<ScriptApplicationHostOptions>>(new OptionsWrapper<ScriptApplicationHostOptions>(applicationHostOptions));
services.AddSingleton<IOptionsMonitor<ScriptApplicationHostOptions>>(new ScriptApplicationHostOptionsMonitor(applicationHostOptions));

services.ConfigureOptions<ScriptJobHostOptionsSetup>();
services.ConfigureOptions<JobHostFunctionTimeoutOptionsSetup>();
// LanguageWorkerOptionsSetup should be registered in WebHostServiceCollection as well to enable starting worker processing in placeholder mode.
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
services.AddOptions<WorkerConcurrencyOptions>();
services.ConfigureOptions<HttpWorkerOptionsSetup>();
services.ConfigureOptions<ManagedDependencyOptionsSetup>();
Expand All @@ -338,8 +337,17 @@ public static IHostBuilder AddScriptHostCore(this IHostBuilder builder, ScriptAp

services.AddSingleton<IFileLoggingStatusManager, FileLoggingStatusManager>();

if (!applicationHostOptions.HasParentScope)
if (applicationHostOptions.HasParentScope)
{
// Forward the host LanguageWorkerOptions to the Job Host.
var languageWorkerOptions = applicationHostOptions.RootServiceProvider.GetService<IOptionsMonitor<LanguageWorkerOptions>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am slightly concerned about how this will work in practice. There are subtleties to the way options are registered and calculated. I would feel better if we did the following:

services.AddSingleton(_ => applicationHostOptions.RootServiceProvider.GetService<IOptionsMonitor<LanguageWorkerOptions>>());
services.AddScoped(_ => applicationHostOptions.RootServiceProvider.GetService<IOptionsSnapshot<LanguageWorkerOptions>>());
services.AddSingleton(_ => applicationHostOptions.RootServiceProvider.GetService<IOptions<LanguageWorkerOptions>>());

Could even lift this to an extension method IServiceCollection ForwardOptionsFrom<TOptions>(this IServiceCollection, IServiceProvider source);

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you expand on the concerns? I like the idea of wrapping that into an extension to make this cleaner and reusable (this is applied to other options). But for the functionality, in this case, we are explicitly trying to avoid another cache miss when using IOptions<T>, for LanguageWorkerOptions specifically, given the cost of running its setup today and the state it tracks (those instances would be identical).

Copy link
Member Author

@fabiocav fabiocav Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, one thing to note is that using the suggested pattern will defer execution of the setup to the time services in the child container request the options, if nothing at the host scope consumes one of those option types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, one thing to note is that using the suggested pattern will defer execution of the setup to the time services in the child container request the options, if nothing at the host scope consumes one of those option types.

That was intentional. I wanted to ensure IOptions in both parent and child container would be the same values. Otherwise, the two containers could potentially have different values if config changes between the two. Unlikely, but that would be one confusing bug if it ever happens.

we are explicitly trying to avoid another cache miss when using IOptions

Would this cache miss not already exist in the parent container?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the pattern currently used in the code, the values would be the same, also refreshed when using IOptionsMonitor, one of the differences is that when using IOptions<T>, you'd end up with the same instance that is returned by IOptionsMonitor<T> (at the initial construction here), but that's intentional.

Would this cache miss not already exist in the parent container?

Right now, there are two cache misses in the JobHost/ScriptHost scope with every single initialization:

  1. Services taking a dependency on IOptionsMonitor<LanguageWorkerOptions>
  2. Services taking a dependency on IOptions<LanguageWorkerOptions>

If no WebHost services are using IOptions<LanguageWorkerOptions> (or if that ever changes in the future as a result of changes to WebHost scoped services), we'd end up with a cache miss when initializing the JobHost, which would have an impact on cold start in some cases (specialization flows shouldn't be as impacted as the expectation is that placeholders would have forced this to happen).

The IOptions<LanguageWorkerOptions> is the second hit we're trying to avoid.

For the WebHost, in specialization cases, even if the host has services consuming IOptions<LanguageWorkerOptions>, those wouldn't impact customer cold start as they would happen in placeholders.

Ultimately, much of what we're dealing with here is the fact the this current setup implementation is doing way too much and that logic should be refactored into a separate, singleton, service that is reused, avoiding the duplicate execution of that expensive logic that is not expected to run more than once anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

The WebHost won't ever be realizing IOptions<LanguageWorkerOptions> when we have a customer payload? If that is the case, then I am fine with the approach as is.

services.AddSingleton(languageWorkerOptions);
services.AddSingleton<IOptions<LanguageWorkerOptions>>(s => new OptionsWrapper<LanguageWorkerOptions>(languageWorkerOptions.CurrentValue));
services.ConfigureOptions<JobHostLanguageWorkerOptionsSetup>();
}
else
{
services.ConfigureOptions<LanguageWorkerOptionsSetup>();
AddCommonServices(services);
}

Expand Down
4 changes: 2 additions & 2 deletions src/WebJobs.Script/Workers/FunctionInvocationDispatcherFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public FunctionInvocationDispatcherFactory(IOptions<ScriptJobHostOptions> script
IHttpWorkerChannelFactory httpWorkerChannelFactory,
IRpcWorkerChannelFactory rpcWorkerChannelFactory,
IOptions<HttpWorkerOptions> httpWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> rpcWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> workerOptions,
IEnvironment environment,
IWebHostRpcWorkerChannelManager webHostLanguageWorkerChannelManager,
IJobHostRpcWorkerChannelManager jobHostLanguageWorkerChannelManager,
Expand All @@ -54,7 +54,7 @@ public FunctionInvocationDispatcherFactory(IOptions<ScriptJobHostOptions> script
eventManager,
loggerFactory,
rpcWorkerChannelFactory,
rpcWorkerOptions,
workerOptions,
webHostLanguageWorkerChannelManager,
jobHostLanguageWorkerChannelManager,
managedDependencyOptions,
Expand Down
48 changes: 44 additions & 4 deletions src/WebJobs.Script/Workers/Rpc/Configuration/LanguageWorkerOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.Azure.WebJobs.Script.Diagnostics;
using Microsoft.Azure.WebJobs.Script.Workers.Profiles;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

Expand All @@ -18,18 +20,21 @@ internal class LanguageWorkerOptionsSetup : IConfigureOptions<LanguageWorkerOpti
private readonly IEnvironment _environment;
private readonly IMetricsLogger _metricsLogger;
private readonly IWorkerProfileManager _workerProfileManager;
private readonly IScriptHostManager _scriptHostManager;

public LanguageWorkerOptionsSetup(IConfiguration configuration,
ILoggerFactory loggerFactory,
IEnvironment environment,
IMetricsLogger metricsLogger,
IWorkerProfileManager workerProfileManager)
IWorkerProfileManager workerProfileManager,
IScriptHostManager scriptHostManager)
{
if (loggerFactory is null)
{
throw new ArgumentNullException(nameof(loggerFactory));
}

_scriptHostManager = scriptHostManager ?? throw new ArgumentNullException(nameof(scriptHostManager));
_configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
_environment = environment ?? throw new ArgumentNullException(nameof(environment));
_metricsLogger = metricsLogger ?? throw new ArgumentNullException(nameof(metricsLogger));
Expand All @@ -42,18 +47,53 @@ public void Configure(LanguageWorkerOptions options)
{
string workerRuntime = _environment.GetEnvironmentVariable(RpcWorkerConstants.FunctionWorkerRuntimeSettingName);

// Parsing worker.config.json should always be done in case of multi language worker
// Parsing worker.latestConfiguration.json should always be done in case of multi language worker
Copy link
Member

Choose a reason for hiding this comment

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

Is worker.latestConfiguration.json a new thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is looking like an accidental change to me. I don't expect a different file name.

if (!string.IsNullOrEmpty(workerRuntime) &&
workerRuntime.Equals(RpcWorkerConstants.DotNetLanguageWorkerName, StringComparison.OrdinalIgnoreCase) &&
!_environment.IsMultiLanguageRuntimeEnvironment())
{
// Skip parsing worker.config.json files for dotnet in-proc apps
// Skip parsing worker.latestConfiguration.json files for dotnet in-proc apps
options.WorkerConfigs = new List<RpcWorkerConfig>();
return;
}

var configFactory = new RpcWorkerConfigFactory(_configuration, _logger, SystemRuntimeInformation.Instance, _environment, _metricsLogger, _workerProfileManager);
// Use the latest configuration from the ScriptHostManager if available.
// After specialization, the ScriptHostManager will have the latest IConfiguration reflecting additional configuration entries added during specialization.
var configuration = _configuration;
if (_scriptHostManager is IServiceProvider scriptHostManagerServiceProvider)
{
var latestConfiguration = scriptHostManagerServiceProvider.GetService<IConfiguration>();
if (latestConfiguration is not null)
{
configuration = new ConfigurationBuilder()
.AddConfiguration(_configuration)
.AddConfiguration(latestConfiguration)
.Build();
}
}

var configFactory = new RpcWorkerConfigFactory(configuration, _logger, SystemRuntimeInformation.Instance, _environment, _metricsLogger, _workerProfileManager);
options.WorkerConfigs = configFactory.GetConfigs();
}
}

internal class JobHostLanguageWorkerOptionsSetup : IPostConfigureOptions<LanguageWorkerOptions>
{
private readonly ILoggerFactory _loggerFactory;

public JobHostLanguageWorkerOptionsSetup(ILoggerFactory loggerFactory)
{
_loggerFactory = loggerFactory;
}

public void PostConfigure(string name, LanguageWorkerOptions options)
{
var message = $"Call to configure {nameof(LanguageWorkerOptions)} from the JobHost scope. " +
$"If using {nameof(IOptions<LanguageWorkerOptions>)}, please use {nameof(IOptionsMonitor<LanguageWorkerOptions>)} instead.";
Debug.Fail(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain the intention behind this? The comment says to use IOptionsMonitor<LanguageWorkerOptions>, but won't this class still run (and fail) for that type?

Copy link
Member Author

Choose a reason for hiding this comment

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

The message is a bit confusing, but with the forwarding, no setups should run for IOptionsMonitor<LanguageWorkerOptions> within the JobHost scope. The same applies to IOptions<LanguageWorkerOptions, though, so either option would take the host injected instances and not run any setup logic within that scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, can we clarify that?

  1. Add a comment explaining this is only registered for JobHost scope
  2. Update debug message to something like "Unexpected configuration of LanguageWorkerOptions from the JobHost scope. LanguageWorkerOptions should be forwarded from the parent scope with no additional configuration."


var logger = _loggerFactory.CreateLogger("Host.LanguageWorkerConfig");
logger.LogInformation(message);
}
}
}
8 changes: 6 additions & 2 deletions src/WebJobs.Script/Workers/Rpc/FunctionRegistration/RpcFunctionInvocationDispatcher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public RpcFunctionInvocationDispatcher(IOptions<ScriptJobHostOptions> scriptHost
IScriptEventManager eventManager,
ILoggerFactory loggerFactory,
IRpcWorkerChannelFactory rpcWorkerChannelFactory,
IOptionsMonitor<LanguageWorkerOptions> languageWorkerOptions,
IOptionsMonitor<LanguageWorkerOptions> workerOptions,
IWebHostRpcWorkerChannelManager webHostLanguageWorkerChannelManager,
IJobHostRpcWorkerChannelManager jobHostLanguageWorkerChannelManager,
IOptions<ManagedDependencyOptions> managedDependencyOptions,
Expand All @@ -87,7 +87,11 @@ public RpcFunctionInvocationDispatcher(IOptions<ScriptJobHostOptions> scriptHost
_webHostLanguageWorkerChannelManager = webHostLanguageWorkerChannelManager;
_jobHostLanguageWorkerChannelManager = jobHostLanguageWorkerChannelManager;
_eventManager = eventManager;
_workerConfigs = languageWorkerOptions?.CurrentValue?.WorkerConfigs ?? throw new ArgumentNullException(nameof(languageWorkerOptions));

// The state of worker configuration and the LanguageWorkerOptions will match the lifetime of the JobHost and the
// RpcFunctionInvocationDispatcher. So, we can safely cache the workerConfigs and workerRuntime here.
// Using IOptionsMonitor here to get the same cached version used by other copmponents and avoid a new instance initialization.
_workerConfigs = workerOptions?.CurrentValue?.WorkerConfigs ?? throw new ArgumentNullException(nameof(workerOptions));
_managedDependencyOptions = managedDependencyOptions ?? throw new ArgumentNullException(nameof(managedDependencyOptions));
_logger = loggerFactory.CreateLogger<RpcFunctionInvocationDispatcher>();
_rpcWorkerChannelFactory = rpcWorkerChannelFactory;
Expand Down
11 changes: 11 additions & 0 deletions src/WebJobs.Script/Workers/Rpc/WebHostRpcWorkerChannelManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public WebHostRpcWorkerChannelManager(IScriptEventManager eventManager,
IMetricsLogger metricsLogger,
IConfiguration config,
IWorkerProfileManager workerProfileManager,
IOptionsMonitor<LanguageWorkerOptions> languageWorkerOptions,
IOptions<FunctionsHostingConfigOptions> hostingConfigOptions)
{
_environment = environment ?? throw new ArgumentNullException(nameof(environment));
Expand All @@ -57,6 +58,16 @@ public WebHostRpcWorkerChannelManager(IScriptEventManager eventManager,
_applicationHostOptions = applicationHostOptions;
_hostingConfigOptions = hostingConfigOptions;

languageWorkerOptions.OnChange(async languageWorkerOptions =>
{
IRpcWorkerChannel rpcWorkerChannel = await GetChannelAsync(_workerRuntime);
if (rpcWorkerChannel != null && !UsePlaceholderChannel(rpcWorkerChannel))
{
_logger.LogInformation("Language worker options changed, and the placeholder worker channel is invalid for other reasons. Shutting down the channel.");
await ShutdownChannelIfExistsAsync(_workerRuntime, rpcWorkerChannel.Id);
Copy link
Member

Choose a reason for hiding this comment

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

What's the scenario here, and why didn't we have this behavior before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will need to review this to comment. That's part of a set of changes pushed by @kshyju .

Changes to the signal used to reload LanguageWorkerOptions were required. I'm concerned that the logic here is assuming this will always change as a result of specialization, which may change in the future.

At the very least, the message needs to be a bit more descriptive.

}
});

_shutdownStandbyWorkerChannels = ScheduleShutdownStandbyChannels;
_shutdownStandbyWorkerChannels = _shutdownStandbyWorkerChannels.Debounce(milliseconds: 5000);
}
Expand Down
Loading