Task to AsyncResult transition wraps Exception into the AggregateException

[xperiandri]

Describe the bug

    member this.AsyncAcquireTokenInteractively (uiThread, correlationId) = asyncResult {
        do! Async.SwitchToContext uiThread
        let builder =
            pca.AcquireTokenInteractive(options.Scopes)
                .WithCorrelationId(correlationId)
        if not (isNull configureInteractive) then configureInteractive.Invoke(builder)
        try return! builder.ExecuteAsync() // FSharp.Control.FusionTasks is used not to write |> Async.AwaitTask
        with :? MsalClientException as ex -> return! Error ex.ErrorCode
    }

Matching by the MsalClientException does not happen as it is wrapped by the AggregateException.

To Reproduce
Steps to reproduce the behavior:

1. Call Task returning method via return! in an asyncResult CE
2. Catch exception
3. You will see that it is always the AggregateException.

Expected behavior
A clear and concise description of what you expected to happen.

Desktop (please complete the following information):

• Version 2.10.0

[TheAngryByrd]

I don't think this is a problem with FsToolkit as this is how exceptions are thrown with Tasks via .NET https://docs.microsoft.com/en-us/dotnet/standard/parallel-programming/exception-handling-task-parallel-library

[xperiandri]

Is it how async CE works itself?

[xperiandri]

It does not happen with await in C#

[xperiandri]

Ah, this is the issue fsharp/fslang-suggestions#840

[xperiandri]

As long as it is a FSharp.Core issue I propose to use a corrected way of awaiting tasks
https://github.com/jet/FsKafka/blob/6c6186f822a221adf0ad06e8a44a392aa55aa8ec/src/FsKafka/Infrastructure.fs#L14 instead of Async.AwaitTask

[TheAngryByrd]

Hey @NinoFloris see any downsides to doing this?

[njlr]

I have this helper in various code-bases:

let rec unpackException (exn : Exception) = 
  match exn with
  | :? AggregateException as agg -> 
    match Seq.tryExactlyOne agg.InnerExceptions with
    | Some x -> unpackException x
    | None -> exn
  | _ -> exn

[bartelink]

As long as it is a FSharp.Core issue I propose to use a corrected way of awaiting tasks https://github.com/jet/FsKafka/blob/6c6186f822a221adf0ad06e8a44a392aa55aa8ec/src/FsKafka/Infrastructure.fs#L14 instead of Async.AwaitTask

Note the edition therein in being synced with the canonical one in jet/FsKafka#92 (similarly fsprojects/FSharp.AWS.DynamoDB#49)

The key specific diff in there is that cancellation will be guaranteed yield an exception, but the more important effect is that people debugging systems can do that based on being able to assume-but-verify identical behavior vs the needless variation having slight variations can cause (especially if we do manage to converge in the end).

[bartelink]

@TheAngryByrd please excuse the atting....

what would you say about a breaking change regarding this?
I recently flipped some taskResult code to asyncResult and fell prey to the silent/implicit change in semantic (i.e. with no longer Doing What I Mean when awaiting something that returns Task<'T>
Possible resolutions are one or more of:

• make asyncResult not magically await tasks (force usage of Async.AwaitTask or something like AwaitTaskCorrect of your choice)
• make the awaits of tasks in this lib do the right thing (inhibit the unhelpful AggregateException wrapping as ATC does)
• put AwaitTaskCorrect into this library and use it internally (though the real answer is to solve it upstream - there's a placeholder issue here: TaskEx: AwaitTaskCorrect / Task.toAsync / Async.ofTask fsprojects/FSharp.Control.TaskSeq#141)

I guess the simplest thing is to remove implicit awaiting? (Checking if there's any way I can inhibit this as a consumer without changing the lib...)

[TheAngryByrd]

@bartelink

I did create an “asyncEx” in IcedTasks with the unwrapping aggregate semantics. Could you give that a once over and make sure it’s working the way you think it should be?

If it does then I think it might be worth making another CE that can polyfill the current one.

Breaking API changes are less scary usually because there’s obvious compile errors but runtime changes does kind of scare me.

[bartelink]

Oh yeah, I remember that now 🤦
Will have a peep (there's another semantic in ATC about honoring cancellation too)
You're not wrong about compile vs runtime semantic changes being a very different level of concern

[bartelink]

Added a comment https://github.com/TheAngryByrd/IcedTasks/pull/24/files#r1556182882

TL;DR I don't think any deviations whatsoever from AwaitTaskCorrect are a good idea.

I don't understand enough about what forces AwaitAwaiter is balancing to be able to comment on the specifics of its implementation - e.g. I don't understand the roles of the various exception mappings etc.

Overall it boils down to two concerns:

• having an implicit await behavior at all
• how many flavours there are in the world of that

For me, there can only be implicit behavior if there's a canonical expectation and/or spec for it. The canonical impl and the associated test snippets on fsssnip provide specific behavior under cancellation too, and that's pretty critical for me

---

I guess this tells us we need to get this resolved in core because having TaskSeq, IcedTasks and/or here all have stuff that's not AwaitTaskCorrect and/or some equivalent with targeted improvements is bad news. I'd so love to stop having to cart it around github.com/jet and FsAwsDdb and/or ever have to compare implementations ever again!

(Ironically, for my work project, having FsToolkit host AwaitTaskCorrect would be the perfect solution, but obviously having the world take a dependency on this entire lib for a correct awaittask impl is a step too far....)

[bartelink]

I think my conclusion/proposal is thus that the thing I'd do is remove the pieces that are bracketed with #if !FABLE_COMPILER for V5:

FsToolkit.ErrorHandling/src/FsToolkit.ErrorHandling/AsyncResultCE.fs

Lines 201 to 217 in dccdbbc

	#if !FABLE_COMPILER
	/// <summary>
	/// Method lets us transform data types into our internal representation.
	/// </summary>
	member inline _.Source(task: Task<'ok>) : Async<Result<'ok, 'error>> =
	task
	|> Async.AwaitTask
	|> Async.map Ok
	/// <summary>
	/// Method lets us transform data types into our internal representation.
	/// </summary>
	member inline _.Source(task: Task) : Async<Result<unit, 'error>> =
	task
	|> Async.AwaitTask
	|> Async.map Ok
	#endif

FsToolkit.ErrorHandling/src/FsToolkit.ErrorHandling/AsyncResultCE.fs

Lines 135 to 142 in dccdbbc

	#if !FABLE_COMPILER
	/// <summary>
	/// Method lets us transform data types into our internal representation.
	/// </summary>
	member inline _.Source(task: Task<Result<'ok, 'error>>) : Async<Result<'ok, 'error>> =
	task
	|> Async.AwaitTask
	#endif

Luckily I'm not the maintainer of the library though, as I'd anticipate people immediately asking me: OK, so where it this wonderful AwaitTaskCorrect thing of which you speak so?

There's also the question of what to replace the usage of Async.AwaitTask in the TryFinallyAsync impl

FsToolkit.ErrorHandling/src/FsToolkit.ErrorHandling/AsyncResultCE.fs

Lines 57 to 93 in dccdbbc

	#if NETSTANDARD2_1
	member inline _.TryFinallyAsync
	(
	computation: Async<Result<'ok, 'error>>,
	[<InlineIfLambda>] compensation: unit -> ValueTask
	) : Async<Result<'ok, 'error>> =
	let compensation =
	async {
	let vTask = compensation ()
	if vTask.IsCompletedSuccessfully then
	return ()
	else
	return!
	vTask.AsTask()
	|> Async.AwaitTask
	}
	Async.TryFinallyAsync(computation, compensation)
	member inline this.Using
	(
	resource: 'ok :> IAsyncDisposable,
	[<InlineIfLambda>] binder: 'ok -> Async<Result<'U, 'error>>
	) : Async<Result<'U, 'error>> =
	this.TryFinallyAsync(
	binder resource,
	(fun () ->
	if not (isNull (box resource)) then
	resource.DisposeAsync()
	else
	ValueTask()
	)
	)
	#endif

On the other hand, having a free-standing net6.0 lib with an asyncEx including neat support for starting cancellableTasks (and awaiting Task/Task<'T>) would be very nice indeed....

And then it begins... you add an asyncExResult eh - this stuff keeps giving eh ;) @TheAngryByrd