Add AsyncLazy<T>.Dispose() method

[AArnott]

This addresses a common need for a class with an AsyncLazy<T> field where the value is disposable to ensure on the class disposal that it can dispose of the value if and when it is constructed.

Problem to be solved

To simplify and optimize a common (and often improperly done) pattern of disposing the value, that would typically appear like this:

class Owner : IDisposable {
  AsyncLazy<IFoo> lazy;

  public void Dispose() {
    if (lazy.IsValueCreated) {
      lazy.GetValue().Dispose();
    }
  }
}

The problems with the pattern above include:

1. They abandon an undisposed value if the value factory is started after disposal.
2. It blocks disposal on an async operation. (IAsyncDisposable owners could avoid the sync block)
3. Folks sometimes forget the condition and construct the value unconditionally.
4. Folks sometimes use IsValueFactoryCompleted, which abandons an undisposed value that has begun construction but isn't done.

Solution

Add a simple Dispose() method on the AsyncLazy<T> class itself that does the right thing, in the most efficient way possible:

1. If the value is already constructed, dispose the value on the caller's thread, if disposable.
2. If the value's construction has already begun but not completed, schedule a task to dispose of the value after construction if the value ends up being disposable.
3. If the value factory has not started yet, disable the value factory, causing any future calls for the value to throw ObjectDisposedException.

Some important design decisions apply and are worth debate prior to merging:

1. Should AsyncLazy<T> itself implement IDisposable?
   I'm leaning on 'no' because that may trigger analyzers to force everyone to dispose of their AsyncLazy<T> objects, even when the constructed value does not itself implement IDisposable.
2. Should GetValue and GetValueAsync methods throw ObjectDisposedException after Dispose is called, or should they just return the disposed value?
   I'm leaning toward throwing at the moment, but returning the disposed value will resemble the existing behavior of folks who are currently disposing of the value by hand. But returning it would mean the value is constructed, only to be disposed of, possibly amplifying race conditions with disposal and a caller's first use of the value. I am on the fence on this one.
3. What should IsValueCreated and IsValueFactoryCompleted return after disposal? Should it vary based on whether the value factory has actually started/completed?
4. Should we add an IsDisposed property?
   Default answer is 'no'.

Alternative design

Expose this as an extension method on AsyncLazy<T> that only appears when T : IDisposable. That would be great, except that quite often T doesn't derive from IDisposable although the constructed value may implement IDisposable. A simple example of this is brokered service proxies, which tend to be declared simply as IFoo but the concrete type also implements IDisposable. As this is a fairly common scenario, I decided to expose the Dispose method regardless of T.

[lifengl]

Yeah, that is a nice function to be added.

[jdrobison]

Some opinions on your debate points:

Should AsyncLazy<T> itself implement IDisposable?
I'm leaning on 'no' because that may trigger analyzers to force everyone to dispose of their AsyncLazy<T> objects, even when the constructed value does not itself implement IDisposable.

I think if AsyncLazy<T> doesn't implement IDisposable, there could be a problem with discovering the new Dispose method. Perhaps that could be addressed by adding a new analyzer to ensure that Dispose is called if T is disposable. Not sure how difficult writing that analyzer would be.

Should GetValue and GetValueAsync methods throw ObjectDisposedException after Dispose is called, or should they just return the disposed value?
I'm leaning toward throwing at the moment, but returning the disposed value will resemble the existing behavior of folks who are currently disposing of the value by hand. But returning it would mean the value is constructed, only to be disposed of, possibly amplifying race conditions with disposal and a caller's first use of the value. I am on the fence on this one.

I like throwing rather than returning the disposed value. You make a fair point about compatibility, but the caller will already have consciously opted into the new behavior by calling Dispose in the first place.

What should IsValueCreated and IsValueFactoryCompleted return after disposal? Should it vary based on whether the value factory has actually started/completed?

IMHO, the behavior of these properties shouldn't change after disposal. They still return information that might be meaningful to the caller, regardless of whether the AsyncLazy<T> itself has been disposed.

---

After writing all of that, I started wondering how my answers would change if the new method was called DisposeValue instead of Dispose:

1. Clearly AsyncLazy<T> wouldn't need to implement IDisposable.
2. It wouldn't make sense for GetValue[Async] to throw ObjectDisposedException, since the AsyncLazy<T> itself can't be disposed.
3. The behavior of IsValueCreated and IsValueFactoryCompleted still doesn't change, but I think the point is no longer up for debate.
4. I suppose there could be some value in an IsValueDisposed. It might even make sense for it to return bool?, where null means that the value was never instantiated, therefore there was nothing to dispose.

AsyncLazy<T> not implementing IDisposable means it can't be used in a using clause or declaration, but I think the need for that would be rare and one can always fall back to try/finally if necessary.

[trippwill]

I agree with @jdrobison's analysis. I like his musings on DisposeValue especially. Explicitness is good since AsyncLazy itself isn't disposable, just that the inner Value might be.

[AArnott]

Thank you for your feedback. In trying to apply @jdrobison's suggestions, it occurs to me that if DisposeValue() is called before the value factory is ever invoked, that nothing will be disposed of, and a subsequent GetValueAsync() call will return an undisposed value. Or we'd create it but immediately dispose of it.
IMO a more reasonable approach is to block access to the value after disposal has been requested so there is no risk of creating a value that should have been disposed of already had it been created.
I'm still thinking through the ramifications of what that means for the rest of the feedback. More comments welcome.

[CyrusNajmabadi]

Fyi, the idea of lifting this aspect of the underlying value to the lazy itself feels pretty wrong to me. Is this something that `Lazy<>` itself does? Also, why would it just be this interface that was lifted in this way? What about things like iequatable, etc.?

…

On Wed, Aug 30, 2023, 14:06 Andrew Arnott ***@***.***> wrote: Thank you for your feedback. In trying to apply @jdrobison <https://github.com/jdrobison>'s suggestions, it occurs to me that if DisposeValue() is called before the value factory is ever invoked, that nothing will be disposed of, and a subsequent GetValueAsync() call will return an undisposed value. Or we'd create it but immediately dispose of it. IMO a more reasonable approach is to block access to the value after disposal has been requested so there is no risk of creating a value that should have been disposed of already had it been created. I'm still thinking through the ramifications of what that means for the rest of the feedback. More comments welcome. — Reply to this email directly, view it on GitHub <#1218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY7TYLP2MKW5ONUOWF3XX6TNBANCNFSM6AAAAAA2JRNQCY> . You are receiving this because your review was requested.Message ID: ***@***.***>

[AArnott]

@CyrusNajmabadi Thanks for reviewing!

The reason we lift it here and not for IEquatable<T> is because IEquatable<T> doesn't own or create a value. AsyncLazy<T> constructs the value and owns it forever after. And T is frequently disposable. Implementing responsible and correct disposal of a async lazily-initialized value is complex, and I'm trying to make that easy to get right.

[CyrusNajmabadi]

What's the difference, philosophically, between this and Lazy<>? Also, are we lifting both IDisposable and IAsyncDisposable up to the lazy itself? Note: I cannot see the actual change as this is in the MS repo and I'm locked out from that.

…

On Wed, Aug 30, 2023, 14:16 Andrew Arnott ***@***.***> wrote: @CyrusNajmabadi <https://github.com/CyrusNajmabadi> Thanks for reviewing! The reason we lift it here and not for IEquatable<T> is because IEquatable<T> doesn't own or create a value. AsyncLazy<T> constructs the value and owns it forever after. And T is frequently disposable. Implementing responsible and correct disposal of a async lazily-initialized value is complex, and I'm trying to make that easy to get right. — Reply to this email directly, view it on GitHub <#1218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY5BNWJN7IPSPLDAFQDXX6URLANCNFSM6AAAAAA2JRNQCY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[AArnott]

I don't know if there is a philosophical difference between Lazy<T> and this one. But I don't own that one, and I am not aware of a common need to dispose a value that hasn't yet been fully constructed by a Lazy<T> either. I'm just looking at usage patterns of AsyncLazy<T> and see a need to address.

[AArnott]

As for supporting IAsyncDisposable, yes I've decided that my next iteration will support that.

[CyrusNajmabadi]

Yeah. I think this just opens too many questions for me. I genuinely am not sure what actual semantics make sense here. Especially for values that are not constructed yet, but get constructed later. Or which may be in the process of being constructed. I'd prefer if all of this was owned by whoever owns the asynclazy itself.

…

On Wed, Aug 30, 2023, 14:27 Andrew Arnott ***@***.***> wrote: I don't know if there is a philosophical difference between Lazy<T> and this one. But I don't own that one, and I am not aware of a common need to dispose a value that hasn't yet been fully constructed by a Lazy<T> either. I'm just looking at usage patterns of AsyncLazy<T> and see a need to address. — Reply to this email directly, view it on GitHub <#1218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MYZFEVNKNSL4CIRQXKDXX6V5PANCNFSM6AAAAAA2JRNQCY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[AArnott]

I'd prefer if all of this was owned by whoever owns the asynclazy itself.

I'm not sure what you mean by that.
I own the class. And the behavior isn't changing unless the owner of the instance opts in by calling the new DisposeValue method.

[CyrusNajmabadi]

I mean: Whoever owns the asynclazy instance can decide what to do based on the existing exposed API. Similar to what they would have to do if this was Lazy<>. I don't like idea of exposing this off asynclazy because it makes it less tenable to easily move to/from Lazy/AsyncLazy if it turns out my computation gets changed to sync/async as my component develops. Anyway, my 2c. It's your API :-)

…

On Wed, Aug 30, 2023, 14:34 Andrew Arnott ***@***.***> wrote: I'd prefer if all of this was owned by whoever owns the asynclazy itself. I'm not sure what you mean by that. I own the class. And the behavior isn't changing unless the owner of the instance opts in by calling the new DisposeValue method. — Reply to this email directly, view it on GitHub <#1218 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY7U3G2HEHH4XZ2JLIDXX6WUXANCNFSM6AAAAAA2JRNQCY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[AArnott]

It's a fair thought, but since roslyn's AsyncLazy<T> class adds CancellationToken support that Lazy<T> doesn't have, your suggestion seems kinda inconsistent with the guidance your team has followed.
And aggregating CancellationToken support here the way Roslyn does is something we're considering porting over, with the idea in mind that maybe roslyn can move over to this version.

[lifengl]

I left a comment, as I still feel it is necessary to add an extra MemoryBarrier inside the code in DisposeValueAsync side.

[jdrobison]

Not sure if nameof(AsyncLazy<T>) is now the right parameter for the ObjectDisposedException, since that's not what's disposed. However, if you tried to smuggle something like nameof(T) into the exception, you'd have a different exception for each T.

[AArnott]

Hmmm... At the moment I'm leaning on going ahead with the AsyncLazy value, since that's what's throwing. It may be odd to throw ODE from AsyncLazy but with a type name that belongs to something else.

[CyrusNajmabadi]

Is the idea that by calling this, a bit is set that indicates that if the value is ever created in the future that it be disposed? So that way there is a guarantee that if ever created it will always be certain to be disposed, and if so that happens at most once? With a lock around the bit and marking the value computed, it feels like this guarantee could be provided. If you'd isn't the guarantee, what are the samantics around asking for the value to be disposed?

…

On Wed, Aug 30, 2023, 15:29 Lifeng Lu ***@***.***> wrote: ***@***.**** approved this pull request. I left a comment, as I still feel it is necessary to add an extra MemoryBarrier inside the code in DisposeValueAsync side. — Reply to this email directly, view it on GitHub <#1218 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABC2MY4AELJ2ZECLOEKVMW3XX65E7ANCNFSM6AAAAAA2JRNQCY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[AArnott]

@CyrusNajmabadi The idea is to make it easy for the AsyncLazy owner to ensure the value is disposed if it were ever created. But it also blocks creation of the value if the value factory hasn't been invoked by that point.