Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Task-based layer #65

Open
bartelink opened this issue Dec 14, 2022 · 2 comments
Open

Support for Task-based layer #65

bartelink opened this issue Dec 14, 2022 · 2 comments

Comments

@bartelink
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants