`MemoryValueEnumerable` Uses `SpanEnumerator` When `MemoryEnumerator` is Expected
Mike-E-angelo opened this issue · 19 comments
I'm not trying to pile on here, I swear! 😅
I am running into this issue:
Error CS8344 foreach statement cannot operate on enumerators of type 'SpanEnumerator<Notification>' in async or iterator methods because 'SpanEnumerator<Notification>' is a ref struct.
I expect this message for Span<T>-based components, but I am using a Memory<T> component which is based on a MemoryValueEnumerable:
I wanted to check in here to make sure this is expected behavior and/or if there are maybe plans to change it. 😁😇
Honestly, I've been needing someone to double-check on my stuff. Of course that I don't like to have errors and neither want to waste your time but, unfortunately, things like this happen.
Let me finish the work on the ArrayPool and then I'll come back here.
No worries, FWIW I am up to my eyeballs in grief with my own re-write, having upgraded to net6.0 along with efcore. It turns out despite my best intentions that I have been performing all my entity and DbContext read/load/write/modify scoped per-user and could/should be doing it as a singleton via IDbContextFactory<T>.
So, the above screenshot is basically going to be the norm for me. It's good as well as I have been also allocating nothing but new arrays and now I have the opportunity to use Memory<T> instead.
This is a limitation that I didn't think of. The async tells the compiler to generate a state machine that handles the awaited calls in the method's body. It must be then trying to assign the enumerator to a field of a class, which is not allowed for ref structs.
There's no issue with synchronous code: https://dotnetfiddle.net/DPboAz
I've been avoiding having to implement a MemoryEnumerator. It means that each call to MoveNext() has to call Memory<>.Span, which is expensive. For this reason, I've been using ArraySegment<> to iterate arrays.
Do you really need to have a Memory<>? What type is lease? Shouldn't it be asynchronous by implementing IAsyncEnumerable<>?
Lease is a simple wrapper around ValueMemoryOwner to help mitigate any friction that may occur from beta development. 😁
Will this go away once the IMemoryOwner changes are implemented? It would be nice to be able to enumerate over a Memory<T> in a performance-minded way (in both synchronous/asynchronous scenarios).
There's no good way to wrap a Memory<> in an enumerable.
I think the best solution is actually your Lease<> type. Can I "steal" the idea?
So, I propose this solution: #380
It adds a Lease<> type: https://github.com/NetFabric/NetFabric.Hyperlinq/blob/Lease/NetFabric.Hyperlinq/Lease.cs
It implements IMemoryOwner<> and is enumerable. It also has the Remaining property, and Length can be set, inspired by your implementation.
ToArray() returns this Lease<> and you can then use foreach directly on its result. For any other case, you can get the Memory<> or the full rented array.
Cool! Any implementation that results in less code that I have to maintain is a good one to me. 😁
It will also warm me up to the idea of using classes instead of readonly structs. :P
The only issue I see is the use of properties for methods. For instance, there is a lot happening with the Memory/Remaining properties that make it tempting to use it as, well, properties. That is, it might be used more than once in a calling method, etc. because it's "just a property." Making it a method makes it seem like it's more costly (which it is). But that's just a personal preference.
Otherwise, looks good. 👍
Oh, the other issue I see is the missing IAsyncDisposable implementation. The reason I added that is that I am doing a LOT of EFCore/asynchronous operations. Well, everything seems asynchronous these days, really.
My understanding is that if you use an IDisposable in an asynchronous context, you run the risk of the IDisposable disposing when the first yield/context switch occurs in the method, which is technically when the method "exits." Is that right? In any case, that is my understanding and why I had it in there.
That said, I totally hate the whole asynchronous setup. In fact, I had a pretty popular vote on MSFT developer community before it got shut down by the powers that be. But this scenario is a good example of why I am not a fan. Total bifurcation of everything and you really have to account for both worlds now. 😞
Oh, the other issue I see is the missing
IAsyncDisposableimplementation. The reason I added that is that I am doing a LOT of EFCore/asynchronous operations. Well, everything seems asynchronous these days, really.
I noticed you had the IAsynsDisposable but didn't understand why as there's no async code when disposing.
My understanding is that if you use an
IDisposablein an asynchronous context, you run the risk of theIDisposabledisposing when the first yield/context switch occurs in the method, which is technically when the method "exits." Is that right? In any case, that is my understanding and why I had it in there.
Based on that, all disposable entities would have to be rewritten. I understand that async/await code is viral but only within the boundaries of true async code.
I wrote a small test using the old-style scoped using pattern. Notice on the right side, in the generated source, that the generated try/finally is a nicely contiguous synchronous chunk of code. I may be wrong, but I think there's no problem here.
If you change it to the new-style implicit scope using pattern, then there are context switches inside the generated try/finally.
Don't you mind using a scoped using instead for the Lease<> type?
I'll merge the #380 PR and release a new package soon.
Ah the issue here is more like what happens when you pass that Lease into another component that has async/await:
(Numbering the sequential execution as I understand it):
class MyOtherDependencyAsync
{
ValueTask<int> Get(Memory<object> parameter); // All sorts of awaiting can occur in here.
}
static async Task TestAsync(MyOtherDependencyAsync instance)
{
await Task.Delay(1);
using (var lease = DetermineLease()) // 1
{
var temp = await instance.Get(lease.Memory); // 3
... // 4
} // 2 - disposed, bad
await Task.Delay(1);
}
If I am not mistaken that Lease will have been disposed, leaving the async with a disposed Lease which means the Memory it is working with may or may not have been cleared/modified when it gets to it.
Maybe I'll write some tests to verify. :P
Goodness @aalmada I am batting .000 here... and more like, there are so many Gotchas with async/await I am now imagining them. 😆
I'm reading this:
https://docs.microsoft.com/en-us/dotnet/api/system.iasyncdisposable?view=net-5.0
And it clearly states that this is intended for long-running disposal processes. I swear I saw a gotcha around disposable blocks, but I cannot find any. So maybe you're correct here and I have a bunch of even more code to fix/change. :P
OK this must be it, in the case of eliding the keywords:
https://stefansch.medium.com/another-set-of-common-async-task-mistakes-and-how-to-avoid-them-23a6af5b1ce8#005d
OK this must be it, in the case of eliding the keywords:
https://stefansch.medium.com/another-set-of-common-async-task-mistakes-and-how-to-avoid-them-23a6af5b1ce8#005d
That makes sense.
using is expanded to a try/finally. If the await is elided, the method is called synchronously, and the possible exception would be thrown after the execution has already passed by the finally.
To my best knowledge, eliding is only safe in two cases:
- When the caller method simply calls the async method.
- If the caller method contains sync code, it has to be before the call to the async method. The
finallyis added after.
Who says bugs are bad? I learn so much from them. 😆
Appreciative of the learning occurring here, @aalmada. 🙏 Or is that re-learning. 😅
I'll close this now since it seems addressed.
I'm getting one of those bugs where the unit test succeeds if I step through the code but not otherwise 😱
Oh noes @aalmada that's right up there with the super rarely failing unit test that you think you fixed but keeps on super rarely failing. I have one with my Lease<T> object, in fact. I think I fixed it? 🤔 It's actually been behaving now for the longest streak yet. Bragging about it here to tempt fate. 😆
Mine is also happening in Lease<>. For some reason it's accessing an instance already disposed. 🤔
I'm going to bed now. I'll look into it tomorrow.
So, we both had the same bug...🤦♂️
Lease<> should be disposed of after usage. Default is a static property, which means it returns a singleton. Well, we were disposing of the same instance multiple times.
I'll fix it tonight... 😰
Doh! Yes, that is a tricky one. Probably should remove that property due to this.