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

[pack] ensuring that we look at the same InStandbyMode value during app initialization #4356

Merged
merged 1 commit into from May 15, 2019

Conversation

Projects
None yet
4 participants
@brettsam
Copy link
Member

commented Apr 26, 2019

Targeted fix for #4355.

Rather than reading from env vars everywhere, which can change during initialization, I've cached them in the options. This way all services will think they're in standby mode and we won't hit partial initialization.

@brettsam brettsam requested a review from fabiocav Apr 26, 2019

@fabiocav
Copy link
Member

left a comment

Need to review any other code using InStandbyMode property in the environment

@brettsam brettsam changed the title ensuring that we look at the same InStandbyMode value during app initialization [pack] ensuring that we look at the same InStandbyMode value during app initialization Apr 26, 2019

@@ -20,6 +20,8 @@ public class ScriptApplicationHostOptions

public string TestDataPath { get; set; }

public bool InStandbyMode { get; set; } = false;

This comment has been minimized.

Copy link
@mathewc

mathewc Apr 29, 2019

Contributor

I don't think we want this to be part of our public options surface area. Can you make this internal? Though unfortunately this is defined in the core Script assembly and configured in WebHost, requiring it to be public.

This comment has been minimized.

Copy link
@brettsam

brettsam Apr 30, 2019

Author Member

I agree that this isn't ideal. This afternoon I tried to break InStandbyMode into its own Options object and make this setup use that instead. But it's introducing some interesting timing issues with the Options cache invalidation (it's happening in the wrong order). Going to think through it again and see if there's another way to break this out...

var hostEnvironment = p.GetService<IScriptWebHostEnvironment>();
if (hostEnvironment.InStandbyMode)
var appHostOptions = p.GetService<IOptionsMonitor<ScriptApplicationHostOptions>>();
if (appHostOptions.CurrentValue.InStandbyMode)

This comment has been minimized.

Copy link
@mathewc

mathewc Apr 29, 2019

Contributor

When should this new value be used v.s. IScriptWebHostEnvironment.InStandbyMode? We still use the latter all over the place. We need clear guidance on when one or the other should be used. Is your new property only to be used during startup service initialization?

This comment has been minimized.

Copy link
@brettsam

brettsam Apr 30, 2019

Author Member

Not only that... we use IEnvironment.IsPlaceholderModeEnabled() in a bunch of places too. We need to do a sweep through here and make sure we're only checking this directly against the environment in very few places. I've filed #4368 to track this work.

_standbyOptions = standbyOptions ?? throw new ArgumentNullException(nameof(standbyOptions));

// If standby options change, invalidate this options cache.
_disposable = _standbyOptions.OnChange(o => _cache.Clear());

This comment has been minimized.

Copy link
@brettsam

brettsam Apr 30, 2019

Author Member

With this latest change -- all the IOptionsMonitor invalidation happens on StandbyOptions. Here, we watch for those changes and invalidate the ScriptApplicationHostOptions cache if we see them. This guarantees ordering.

@brettsam

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

@fabiocav / @mathewc -- take a look at my latest change. It addresses Mathew's concern but at the sake of more complicated wire-up. In order to get things firing in the correct order, I needed to make sure the StandbyOptions cache was invalidated first and then the ScriptApplicationHostOptions can react to that by invalidating its own.

I kept this a separate commit so I can roll it back if we decide it's not worth it...

@fabiocav
Copy link
Member

left a comment

Couple of nit comments.

var hostEnvironment = p.GetService<IScriptWebHostEnvironment>();
if (hostEnvironment.InStandbyMode)
var standbyOptions = p.GetService<IOptionsMonitor<StandbyOptions>>();
if (standbyOptions.CurrentValue.InStandbyMode)

This comment has been minimized.

Copy link
@mathewc

mathewc May 14, 2019

Contributor

If the options are initialized as InStandbyMode=False and earlier in the pipeline we initialize temp placeholder paths, and before we get here we are specialized (and the options are reset), doesn't the same bug exist, since we're getting the current value rather than using the initial value?

This comment has been minimized.

Copy link
@brettsam

brettsam May 14, 2019

Author Member

Ah, I remember now. The only way that this value can be reset is if the StandbyManager resets it -- and that can only happen if the host has already started (it's in an IHostedService). So we know it won't change during initialization, unlike the environment variables, which can change at any time.

This comment has been minimized.

Copy link
@mathewc

mathewc May 14, 2019

Contributor

Ok. Recommend adding those details to the summary comments I asked for above :)

This comment has been minimized.

Copy link
@fabiocav

fabiocav May 14, 2019

Member

Agreed.

This comment has been minimized.

Copy link
@brettsam

brettsam May 14, 2019

Author Member

👍

@brettsam brettsam force-pushed the brettsam:place_race branch from 4ba4735 to 8589c07 May 14, 2019

@brettsam brettsam force-pushed the brettsam:place_race branch 2 times, most recently from 7ba243f to 5b73150 May 15, 2019

@brettsam brettsam force-pushed the brettsam:place_race branch from 5b73150 to f4f899b May 15, 2019

@brettsam brettsam merged commit 04005b9 into Azure:dev May 15, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.