dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.15k stars 4.71k forks source link

OptionsCache fails to recover from initialization failure #41863

Open RaffaelHold opened 4 years ago

RaffaelHold commented 4 years ago

Description

When passing a factory method - that might throw a runtime exception - as a parameter to .GetOrAdd() on an instance of OptionsCache the cache will never recover if an exception gets thrown in the factory method.

This happens because the underlying Lazy will never try to reinitialize its value even if the error condition goes away.

For example Microsoft.Identity.Web does make an remote HTTP call during initialization of JwtBearerOptions. If theres a connection outage, when the factory method is executed, every subsequent cache access will throw an exception.
The only fix is to manually clear the cache or restart the application.

Sample:

using System;
using Microsoft.Extensions.Options; // Microsoft.Extensions.Options @ 3.1.7

namespace ConsoleApp
{
    public static class Program
    {
        static void Main()
        {
            var optionsCache = new OptionsCache<Options>();
            try
            {
                var expectedToFail = optionsCache.GetOrAdd("options", () => new Options());
            }
            catch { }

            var shouldBeSucceeding = optionsCache.GetOrAdd("options", () => new Options());
        }

        private class Options
        {
            private static bool _throws = true;
            public Options()
            {
                if (_throws)
                {
                    _throws = false;
                    throw new Exception();
                }
            }
        }
    }
}

Possible Solutions

a) change the Lazy's LazyThreadSafetyMode to PublicationOnly to allow multiple evaluations until one succeeds (this might have unintended performance consequences)
b) reset the cache entry when an exception is thrown during initialization of the Lazy

Configuration

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 4 years ago

Tagging subscribers to this area: @maryamariyan See info in area-owners.md if you want to be subscribed.

ViktorHofer commented 4 years ago

@RaffaelHold are you sending the HTTP request during the construction of your Options instance?

RaffaelHold commented 4 years ago

@ViktorHofer I found this issue in Microsoft.Identity.Web where indeed an HTTP request is sent during construction of the Options instance. But any runtime exception will lead to this behavior

Here are the relevant lines in Microsoft.Identity.Web Microsoft.Identity.Web/WebApiExtensions/WebApiAuthenticationBuilderExtensions.cs which leads to a HTTP call being made in AadIssuerValidator.GetIssuerValidator

I hope this gives you a better idea what's happening.

I'd also be happy to give the implementation of a fix a shot if I get some pointers on how we want to resolve this issue.

davidfowl commented 4 years ago

Don't make http requests that block during construction of objects. Do it before or after. This could even result in deadlocks if not careful.

Even if the lazy doesn't re-evaluate, this looks like a bad idea and it's weird that options would be re-initialized and retired. Constructions of objects isn't something that should be unreliable and retried (or bake the retries into the code itself)