Introduce a fourth type of LazyThreadSafetyMode: ThreadSafeValueOnly

[mariusGundersen]

(This is opened based on feedback from a pull-request to coreclr)

This is a request to enhance the LazyThreadSafetyMode enum and the Lazy<T> class to support a fourth scenario.

Rationale and Description

There seems to be a missing LazyThreadSafetyMode that is commonly asked for (for example), which would behave sort of as a mix of ExecutionAndPublication and PublishOnly. This mode is similar to ExecutionAndPublication in that it uses locks so only a single thread can run the factory at a time, and other threads will wait until the first thread is done. But it is different in that it does not cache exceptions, only successful values, similar to what PublicationOnly does. If the value factory throws an exception then the Lazy<T> remains in the pending state, so that the next time the Value is accessed it will call the factory again.

Usecases

This is very useful when Lazy<T> is used for values that are expected to fail intermittently but recover quickly. For example, it might be used to store the result of a network request triggered by incoming requests to a web server. All of these need to be satisfied in this scenario:

• The request should not be made until it is actually needed (therefore Lazy<T> is used)
• The result should be cached forever
• There may be multiple threads that need the value at the same time (therefore locking is needed)
• The request can fail, since it involves network IO.
• Each thread is independent; what happens to one should not affect the other

There currently isn't any simple way to do this without having to rewrite Lazy<T> completely from scratch. Luckily it is not very difficult to extend it to support this scenario

Potential implementation

Having looked at the code it seems like the only change needed is to not store the exception when it is thrown, so that the next call finds the Lazy<T> in the initial pending state. The simplest way to change Lazy<T> to behave this way is to remove this line. Obviously it shouldn't be changed, but enhanced, and so a new mode is needed that is identical to the code path of the ExecutionAndPublication mode, except for missing this single line. I have attempted to do this in the pull-request to coreclr to coreclr.

Discussion

The name ThreadSafeValueOnly is very much bikeshedable, so if you have a suggestion for another name of this mode I am very much up for changing it.

[danmoseley]

@stephentoub

[stephentoub]

stephentoub

Seems reasonable functionally, albeit with a better name 😄. Maybe ExecutionAndPublicationValueOnly, ValueExecutionAndPublication, ExecutionAndPublicationSuccess, or something along those lines. As long as it's consistent with the other current names (None, PublicationOnly, and ExecutionAndPublication), should be fine.

[danmoseley]

Shall we flip this to ready-for-review and we can discuss naming there?

[GSPP]

This is very important to have. If a lazy instance is used as a cache in a web application and it's execution fails then the web app is permanently down. Many things worth caching can fail transiently (any database or web service call). The ExecutionAndPublication mode is fundamentally unsafe in many scenarios.

The weaker modes are not very nice to use in caching scenarios because they can cause cache stampeding due to multiple executions.

[stephentoub]

Thinking about this more, I'd prefer a slightly different design. There's nothing in the LazyThreadSafetyMode per se that's about whether exceptions are cached or not; although the XML docs go into details about resulting behavior, the values are really just about the thread-safety nature of the operation. As such, whether exceptions are cached or not could actually apply to any of the existing values, and it would be better if that option were thusly applicable to all of them. This would give the ability for both None and ExecutionAndPublication to not cache the exception (today they do), and for PublicationOnly to cache the exception (today it doesn't). So, I'd prefer to see this exposed as one or more new ctors rather than as a new value in LazyThreadSafetyMode, e.g.

public Lazy(Func<T> valueFactory, LazyThreadSafetyMode mode, bool storeExceptions);

[mariusGundersen]

There's two reasons I added a new enum value rather than add a boolean argument:

First of I'm of the opinion that adding bools as arguments is bad for readability of the code, and that it is better to use enums or have multiple methods. This is well documented and is a pattern I've seen other places in C#, for example in Index Of, which could have taken a boolean for case invariance but instead uses an enum with several values.

And second is that I don't think features should be added unless they are needed. Yes, the other options could use a flag for toggling caching of exceptions too, but is there a real demand for that? It would just make the already complex Lazy even more complex, maybe not much in code but definitely in understanding all the possible combinations and which one is the correct one for the situation at hand.

[stephentoub]

I understand your reasons. But whether the exception is cached or not has little to do with the thread safety mode of the object, which is what that enum is about. From my perspective, adding an unrelated and orthogonal value to an enum is more confusing and more complex than adding it as a separate argument. In the IndexOf example, case is a part of how the string comparison happens, so it makes sense as part of something name "StringComparison"; that's not the case here.

[mariusGundersen]

I see what you mean. It's a bit weird that two of the LazyThreadSafetyMode enum values cache the exception but one of them don't, so the bool final argument will have a strange definition for its default value. IMO the LazyThreadSafetyMode enum is strangely named and Lazy is already a bit too complicated to use, and probably should have been multiple different classes, but that's too late to fix, so it's not really possible to enhance it without making it even more complex. I am still not fond of bools as the final argument, and would prefer another separate enum for readability of code. It's hard to know what true means in a list of arguments, it's easier to understand what ExceptionStrategy.Retry means and what the called method will do. But, I'm more interested in having this feature than to get tied up in api design discussions, so I can live with the bool parameter 😄

[stephentoub]

will have a strange definition for its default value

There wouldn't be a default value.

It's hard to know what true means in a list of arguments

Thankfully we have named arguments now, e.g.

new Lazy<T>(..., storeExceptions: true);

[mariusGundersen]

'Default value' as in one of the overloads that don't take the bool argument. Most of the time when there is an overload without a bool argument it's clear how it will behave, as if there was a default value, but in this case it might be more difficult to document clearly.

[mariusGundersen]

But I'm not objecting to your suggestion, so if there is a vote or something now (I've never proposed something here before) it has my vote 👍

[svick]

If the example use case is caching the result of a web request, doesn't that mean adding this would promote waiting synchronously for IO operations?

And I think the pattern of using Lazy<Task<T>> for this purpose, as suggested by @stephentoub in an article from 2011, wouldn't work well here, because the Task will be produced successfully, even if the request fails, so the failing Task would still be cached by Lazy.

[GSPP]

Async support for Lazy would be very useful. I have added a ticket for that: Add async support to System.Lazy dotnet/corefx#32552.

[mariusGundersen]

@stephentoub, what is the status of this?

[stephentoub]

Nothing has changed. It's still marked as api-suggestion, and the design was still being debated.

[mariusGundersen]

Since nothing has changed here I have made a very simple implementation of a Lazy for my own code, an implementation that only has one mode:

public class AtomicLazy<T>
{
    private readonly Func<T> _factory;

    private T _value;

    private bool _initialized;

    private object _lock;

    public AtomicLazy(Func<T> factory)
    {
        _factory = factory;
    }

    public AtomicLazy(T value)
    {
        _value = value;
        _initialized = true;
    }

    public T Value => LazyInitializer.EnsureInitialized(ref _value, ref _initialized, ref _lock, _factory);
}

[ezgambac]

Any update on this? It would be pretty useful to have either a no cache exception mode, or at a least a property to check if the Lazy threw when created, right now IsValueCreated can mean either Lazy not initialized or Lazy tried to initialized but threw.
At least a ExceptionOnInit flag would be nice.

Then you could have something like:

if (lazy.ExceptionOnInit)
{
lazy = new Lazy(factory);
}

[theodorzoulias]

Since nothing has changed here I have made a very simple implementation of a Lazy for my own code, an implementation that only has one mode:

@mariusGundersen this is a decent implementation, but I think that a more desirable behavior would be to propagate the exception to all the threads that are currently waiting. A thread should not have to wait for longer than the duration of a single factory invocation.

I've posted an alternative implementation here (LazyWithRetry<T>), along with a demonstration of the difference in behavior between that and the AtomicLazy<T>.

[akirayamamoto]

Any updates here? Only the value factory constructor caches exceptions by default, this feels like a trap. It would be nice to at least have an option to not cache exceptions.

[verdie-g]

With @stephentoub's comment, the proposal would be

public class Lazy<T>
{
+    public Lazy (Func<T> valueFactory, System.Threading.LazyThreadSafetyMode mode, bool storeExceptions);
}

I agree that the bool is not ideal here especially that some other constructors already take a bool with a different meaning (public Lazy (bool isThreadSafe)) but I think it's better than adding LazyThreadSafetyModes as it's a different dimension, i.e. most combinations storeExceptions X LazyThreadSafetyMode are acceptable.

If we're okay with that proposal I could be interested implementing that change.

[verdie-g]

Instead of the boolean we could also add a new enum

public class Lazy<T>
{
+    public Lazy (Func<T> valueFactory, System.Threading.LazyThreadSafetyMode mode, System.LazyOptions options);
}
+ 
+ [System.Flags]
+ public enum LazyOptions
+ {
+     None = 0,
+     /// <summary>
+     /// Avoid catching and storing the exception the initialization function could throw when calling
+     /// <see cref="Lazy{T}.Value" />. In that case, the exception is thrown, nothing is stored, and the next call
+     /// to <see cref="Lazy{T}.Value" /> would call the initialization function again.
+     /// </summary>
+     /// <remarks>This option is redundant when the thread safe mode is <see cref="LazyThreadSafetyMode.PublicationOnly" />.</remarks>
+     ThrowInitializationExceptions = 1,
+ }

LazyThreadSafetyMode is in System.Threading but Lazy is in System, so I suppose LazyOptions should be in System too.