Refactor SelectAsync to use Try<T>

[Arkatufus]

No description provided.

[Arkatufus]

Self-review

[Aaronontheweb]

LGTM

[tomachristian]

Hey guys, I think this is not a struct tearing problem. I will copy here what I observed and also noted here: https://discord.com/channels/974500337396375553/974500337954222133/1352046946407153745.

I managed to reproduce this bug (on our code-base) and actually managed to also get the debugger involved. I had to do a LOT of iterations to get here:

So, the problem actually is that these are the element types flowing through that SelectAsync:

one of which is a Vogen value-object... WHICH, in case it is default, equality always yields false, as proven by:

This actually tells us that the problem is two fold:

• the fact that Akka is doing equality on the entire NotYetThere (would have worked if it did that only on .Exception)
• the fact that we have this strange type that does not behave properly w/regards to equality on default (it would have worked if it DID satisfy equality for default)

⚠ BUT this also tells us this is NOT a struct tearing problem.

IMO, you guys could easily go ahead with the alternate fix where you keep the Result<T> struct use AND switch to checking reference equality on .Exception instead.

This is the entire code needed to reproduce it 100% of the time:

public readonly record struct IAmARareStructThatDoesNotRespectEquality
{
    public bool Equals(IAmARareStructThatDoesNotRespectEquality _) => 
        false;
}

var actorSystem = ActorSystem.Create("Foo");

await Source.Repeat(1)
    .Select(_ => new IAmARareStructThatDoesNotRespectEquality())
    .SelectAsync(int.MaxValue, async i =>
    {
        await Task.Delay(ThreadLocalRandom.Current.Next(0, 10000));
        return i;
    })
    .RunAggregate(0, (t, _) => t, actorSystem);

I agree that this is a VERY rare case, but not rare enough that we were able to "avoid" it.

[to11mtm]

@tomachristian need to look closer but what about a struct like this help? https://github.com/akkadotnet/akka.net/pull/7028/files#diff-14e21d13541082820f403edfa36d29c58688f2a3e45d5ea97978b9ce5be41456

Main reason I bring it up is, this is a bit more compact than Try while having clearer semantics for the case of results/etc...

[tomachristian]

@to11mtm you mean to replace our ValueTuple usage and avoid this problem? Yes of course, now knowing the actual root cause we have various ways to avoid/workaround this problem in our code-base, no problem.

But IMO, still, this should be fixed in Akka and rely on the .Exception equality of the Result<T> struct (its "maybe" more performant to than a ref type like Try).

[Aaronontheweb]

@to11mtm you mean to replace our ValueTuple usage and avoid this problem? Yes of course, now knowing the actual root cause we have various ways to avoid/workaround this problem in our code-base, no problem.

But IMO, still, this should be fixed in Akka and rely on the .Exception equality of the Result<T> struct (its "maybe" more performant to than a ref type like Try).

Yep, this is a good argument - perserves perf (there is no boxing with our stuff currently, AFAIK)

[to11mtm]

@to11mtm you mean to replace our ValueTuple usage and avoid this problem? Yes of course, now knowing the actual root cause we have various ways to avoid/workaround this problem in our code-base, no problem.
But IMO, still, this should be fixed in Akka and rely on the .Exception equality of the Result<T> struct (its "maybe" more performant to than a ref type like Try).

Yep, this is a good argument - perserves perf (there is no boxing with our stuff currently, AFAIK)

There is some that may be happening depending on specifics (Interpreter/fusing comes to mind, see #7116) but ironically, getting rid of boxing by converting 'very' nongeneric methods into generics is sometimes counterintuitive due to the code complexity cost of reification.... (OTOH, ValueTuples are overall a little suss because equality operators can result in unintended refiication cost, doubly so when you -don't- need equality on the item itself...)

tl;-dr; SlimResult may be more performant than current Result struct due to padding/alignment/etc based on usage but should be measured, and it will not eliminate boxing necessarily.

Overall I think SlimResult is probably better than current result struct (We could always introduce it as new API, that would also make it easier to transition for internal and users) but boxing in interpreter/etc is a whole hill of beans, 7116 showed we could lower alloc on boxing in streams (possibly more relevant for specific cases but still good for enough common cases to consider...) perf is very dependent on pool strategy however...

---

Sorry., probably went too far, that said AFAIK Reference-reference equality checks are not going to have a 'torn' read risk, vs the current state of Result, and while we do have boxing it may be tricky to eliminate...

Heck, if one wanted to be extra 'lazy/paranoid' they could instead have null exception (i.e. no sentinel) on SlimOption trigger a spinwait and/or full fense.... ugly but may help sniff out any other not-quite-conforming things in streams implementations...

[Aaronontheweb]

What is SlimResult ?

[Aaronontheweb]

closed via #7543

[to11mtm]

What is SlimResult ?

It's part of my (now stale ;_;) #7028 PR. I linked in prior comment to the struct in question (i.e. 'would something like this help') but here's a link to keep it simple: https://github.com/akkadotnet/akka.net/pull/7028/files#r1496813859

[Aaronontheweb]

What is SlimResult ?

It's part of my (now stale ;_;) #7028 PR. I linked in prior comment to the struct in question (i.e. 'would something like this help') but here's a link to keep it simple: https://github.com/akkadotnet/akka.net/pull/7028/files#r1496813859

Help me get the PR validation pipeline to succeed with a 95% pass rate on PRs that run the full solution build - and after that I might consider merging in something that complicated :p