fsprojects / FSharp.AWS.DynamoDB

F# wrapper API for AWS DynamoDB
MIT License
58 stars 18 forks source link

Support for Task-based layer #65

Open bartelink opened 1 year ago

bartelink commented 1 year ago

Logging this as a placeholder - it's more of a nice to have than something that I'd be personally investing the time to execute on at present.

In Equinox and Propulsion, I've recently transitioned to task from async. This means Equinox.DynamoStore has a layer which is all task and/or TaskSeq stuff, which then needs to call out to FSharp.AWS.DynamoDB. Aside from the perf cost, the layers of redundant mapping also make it harder to diagnose/reason about exception mappings and the like.

If one was building this lib today, arguably one would build it primarily with task in the first instance.

For Equinox's purposes, the ideal would be to introduce a layer that exposes a Task-based API, and then layer the existing one over that. I suspect that over time there will be a growing number of usage scenarios where the caller is task-based.

One concern would be that realistically one would need to expose a ?ct (or perhaps a non-optional CancellationToken arg) absolutely everywhere ... unless one waits for a long term cancellableTask impl to gain currency).

samritchie commented 1 year ago

I agree I think it’s time we should be adding (or replacing with) task. I’m not sure how many consumers would still be using async, would it be unreasonable to ask them to add Async.AwaitTask everywhere (or stick with the older version)?SQLProvider appears to have gone full task about a year ago in a breaking change.

bartelink commented 1 year ago

It's 0.x so I agree doing the right thing soon, (including considering triggering light breaking changes) would be for the best.

As mentioned in the OP, main thing I and presumably others would look for is ct arguments - I'd strongly consider making those mandatory (can always be passed CancellationToken.None) as the default task builder will silently let you invoke a Task-returning function without propagating the CT (i.e. an invocation of a given FSharpAsync function from an async that previously implicitly propagated the CT does not give any nudge that something is being lost).

But, FWIW, I still think async makes sense for apps, the ugliness of propagating cts correctly is far from fun.

It is definitely nice to, as you currently can, be able to eliminate all invocations of FSAWSDDB calls from your inquiries in one fell swoop wrt whether cancellation is wired correctly. (Though, when calling the current API from a task { expr, the current builder impl doesn't prompt you in any way about threading through cancellation to the FSharpAsync API, which is unfortunate IMO)

The other elephant in the room is that the default semantics of Async.AwaitTask are problematic, something that FSAWSDDB does a good job of encapsulating. Replacing Async APIs with Task ones then means the caller would need to take on the responsibility of honoring cancellation.