Skip to content

Add let! and do! support for F# async / Async<'T> #114

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

Merged
merged 5 commits into from
Dec 18, 2022

Conversation

abelbraaksma
Copy link
Member

@abelbraaksma abelbraaksma commented Nov 28, 2022

Fixes: #79

This introduces async support similar to how task supports async through let-bang and do-bang.


Original PR contained the following, which has been moved to #132 and #133.

I'm not certain whether or not to allow this (creative?) approach of setting a CancellationToken.

Basically, if we'd merge this, setting a CancellationToken can be done as follows:

let f() =
    let tokenSource = CancellationTokenSource(500)
    taskSeq {
        do! tokenSource.Token // this sets the token for this taskSeq
        do! Task.Delay 300
        do! Task.Delay 300
        do! Task.Delay 300
    }

@dsyme, what do you think? In a way I think it is smoother than adding overloads to all the TaskSeq.XXX functions to take a cancellation token. Alternatively, of course, I could just have a TaskSeq.setCancellationToken method. However, the above way is very flexible.

The relatively simple way this is done is as follows:

member inline _.Bind(myToken: CancellationToken, continuation: (unit -> TaskSeqCode<'T>)) : TaskSeqCode<'T> =
    TaskSeqCode<'T>(fun sm ->
        sm.Data.cancellationToken <- myToken
        (continuation ()).Invoke(&sm))

Which may also be a way to allow cancellation tokens to be passed through in tasks (but, it adds overhead).

@abelbraaksma abelbraaksma force-pushed the add-async-support branch 2 times, most recently from 2bf9594 to 542472a Compare December 6, 2022 10:36
@abelbraaksma
Copy link
Member Author

/cc @dsyme what do you think?

@abelbraaksma
Copy link
Member Author

@bartelink would you have an informed opinion on this method of setting the cancellation token?

@abelbraaksma abelbraaksma self-assigned this Dec 15, 2022
@abelbraaksma abelbraaksma added enhancement New feature or request topic: taskseq-ce Related to the taskseq computation expression builders or overloads labels Dec 15, 2022
@abelbraaksma abelbraaksma marked this pull request as draft December 15, 2022 01:12
@bartelink
Copy link
Member

bartelink commented Dec 15, 2022

I'm at the lower end of the 'informed' spectrum on this, but won't let that stop me opining...

People could cope with this, and it's obviously terse. It's also arguably a dual to the do! fun ct -> ... construct in IcedTasks.

But, to my eyes, it's not terribly searchable or scannable in the same way that e.g. Async.CancellationToken clearly is; it took me a few takes to grok it on first glance when I first saw it. Hence I'd personally go for a TaskSeq.setCancellationToken API over supporting this.


However, this also begs questions like - would you ever want to setCT more than once? I can't think of any case where I'd only become aware of/generate the CT other than immediately when starting a Task?

Is there any way the CT can become an argument to the builder? e.g. if the code could become taskSeqCt tokenSource.Token {, that would convey the intent (it's at its heart a cancellableTask) more clearly, and generally align with the convention of supplying a ct argument whenever a Task is being kicked off. While that name/letter sequence is probably a tough sell, it is searchable and easy to scan.

Even if there was still a get out of jail card in the form of a TaskSeq.setCancellationToken API, one could validate correct flowing of CTs in a given codebase by looking for either taskSeqCt, or taskSeq + TaskSeq.setCancellationToken pairs.


for posterity, answers & follow up discussion: see #133.

@abelbraaksma
Copy link
Member Author

Hence I'd personally go for a TaskSeq.setCancellationToken API over supporting this.

that’d most certainly be added as well. All CE methods are also supported in point free style.

@gusty
Copy link
Member

gusty commented Dec 16, 2022

Why not using a CE Custom Operator for that ?

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Dec 17, 2022

Why not using a CE Custom Operator for that ?

EDIT: continued in PR #132.

@gusty That would be my preference. Less magic, more explicitness. But last time I tried to mix "default" (yield!/let! etc) styles with ones that have custom operators, it all fell apart (but, to be fair, that was trying to add let!/do! behavior into something that only had custom operations, i.e. the other way around and I probably just screwed something up).

I didn't try it this time around, I'll give it a go.

EDIT: tried it, failed it. I probably do something wrong here. Since there's zero docs on custom operations (apart from a few lines of explanatory text) I always have to figure it out by just trying.

It compiles fine, but when I use cancellationToken it says that cancellationToken is not defined:

        [<CustomOperation("cancellationToken")>]
        member inline _.SetCancellationToken(myToken: CancellationToken, continuation: (unit -> ResumableTSC<'T>)) : ResumableTSC<'T> =
            ResumableTSC<'T>(fun sm ->
                sm.Data.cancellationToken <- myToken
                (continuation ()).Invoke(&sm))

This does not compile:

taskSeq {
    cancellationToken (CancellationToken())
    yield! [ 1..10 ]
 }

It's also unfortunate that you need the extra parentheses...

@gusty do you know if this can be fixed or is this a limitation of the language? Or perhaps a limitation of trying custom operations with resumable code?

EDIT: see #132.

@abelbraaksma abelbraaksma force-pushed the add-async-support branch 3 times, most recently from 2bf9594 to 9cb0ca4 Compare December 17, 2022 01:59
@abelbraaksma abelbraaksma changed the title Add let! and do! support for async and allow setting of CancellationToken Add let! and do! support for async ~and allow setting of CancellationToken~ Dec 18, 2022
@abelbraaksma abelbraaksma changed the title Add let! and do! support for async ~and allow setting of CancellationToken~ Add let! and do! support for F# async / Async<'T> Dec 18, 2022
@abelbraaksma
Copy link
Member Author

abelbraaksma commented Dec 18, 2022

I shouldn't have conflated adding these two features in one PR. So I'm gonna split this up:

I've addressed the relevant questions there.

Will continue this one with just the simple task of adding async support.

@abelbraaksma abelbraaksma marked this pull request as ready for review December 18, 2022 03:12
@abelbraaksma abelbraaksma requested a review from gusty December 18, 2022 14:35
@abelbraaksma abelbraaksma merged commit 6be76b0 into main Dec 18, 2022
@abelbraaksma abelbraaksma deleted the add-async-support branch December 18, 2022 14:55
@abelbraaksma
Copy link
Member Author

This will be published as part of 0.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request topic: taskseq-ce Related to the taskseq computation expression builders or overloads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interoperating with existing F# async
3 participants