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

Affine tasks #107

Merged
merged 4 commits into from Nov 18, 2020
Merged

Affine tasks #107

merged 4 commits into from Nov 18, 2020

Conversation

Swoorup
Copy link
Contributor

@Swoorup Swoorup commented Nov 17, 2020

Fixes #106

along with few other fixes

  • Use affine tasks
  • Remove side by side github workflow hack

@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 17, 2020

@NinoFloris any reasons why methods like task.Return from ply return Ply type instead of tasks?

@TheAngryByrd
Copy link
Collaborator

Ugh that side-by-side hack isn't working anymore 😢

@TheAngryByrd
Copy link
Collaborator

Ugh that side-by-side hack isn't working anymore 😢

Oh nice it was actually fixed and here's an example https://github.com/actions/setup-dotnet#usage

@NinoFloris
Copy link
Contributor

@Swoorup this is just the 'internal' exchange type, same goes for Taskbuilder (which has Step), it's the lowest common denominator between ValueTask and Task.

@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 17, 2020

Should fstoolkit be using ply as intermediate type at all, or always use task? I am finding it hard to work out compiler messages. The test are failing to build but the library builds. Maybe it should thinking from your reply

@Swoorup Swoorup force-pushed the sj-affine-tasks branch 2 times, most recently from 07ce5de to f2924f3 Compare November 18, 2020 14:14
@Swoorup Swoorup marked this pull request as ready for review November 18, 2020 14:14
@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 18, 2020

It appears FAKE doesn't allow changing the lang target to net50. Wrong value duh.

Other than that this is ready for review. @TheAngryByrd

@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 18, 2020

Not sure how to fix the tooling issue
FSharp.Compiler.CompileOps+FileNameNotResolved' was thrown

@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 18, 2020

Reverted to just fix the linked issue

Copy link
Collaborator

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @NinoFloris wanna give it a once over as well?

Copy link
Contributor

@NinoFloris NinoFloris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch looks good.

I'm not quite sure about all the renames from uply to task, as the return type that you get is not Task but Ply, it doesn't really aid readability this way but I'm not against it either.

One important change that should happen is every place where uply { ... } was swapped for task { ... } |> task.ReturnFrom should get reverted, this adds needless speedbump allocations going to Task + methodbuilders etc to then go straight back to Ply.

The idea of uply {} is for it to be used as an internal type of the CE methods as much as possible, only to be mapped to the final type Task/ValueTask/Async in Run.

Either way, thanks for doing this :)

@TheAngryByrd I'm all for this change but there was a reason we didn't do this during the conversion to Ply in the first place. It should be very clear to users of the library currently depending on these CEs to automatically apply ConfigureAwait(false) that they can no longer expect it to do so. Meaning at every bind where they previously expected the CE to ignore any sync context or task scheduler it's now up to users to sprinkle ConfigureAwait(false) on those tasks before binding. If not then this release is bound to create deadlocks for users of gui or older aspnet frameworks.

src/FsToolkit.ErrorHandling.TaskResult/TaskOptionCE.fs Outdated Show resolved Hide resolved
src/FsToolkit.ErrorHandling.TaskResult/TaskOptionCE.fs Outdated Show resolved Hide resolved
src/FsToolkit.ErrorHandling.TaskResult/TaskOptionCE.fs Outdated Show resolved Hide resolved
src/FsToolkit.ErrorHandling.TaskResult/TaskResultCE.fs Outdated Show resolved Hide resolved
src/FsToolkit.ErrorHandling.TaskResult/TaskResultCE.fs Outdated Show resolved Hide resolved
src/FsToolkit.ErrorHandling.TaskResult/TaskResultCE.fs Outdated Show resolved Hide resolved
@TheAngryByrd
Copy link
Collaborator

@TheAngryByrd I'm all for this change but there was a reason we didn't do this during the conversion to Ply in the first place. It should be very clear to users of the library currently depending on these CEs to automatically apply ConfigureAwait(false) that they can no longer expect it to do so. Meaning at every bind where they previously expected the CE to ignore any sync context or task scheduler it's now up to users to sprinkle ConfigureAwait(false) on those tasks before binding. If not then this release is bound to create deadlocks for users of gui or older aspnet frameworks.

Yeah I'm sure I'm gonna get an issue about this problem too. Might have to make a separate package with unaffine vs affine but we'll see if people complain first before I go fixing problems that may not exist. Additionally the older versions aren't going anywhere for now.

Copy link
Collaborator

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Nino's comments.

@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 18, 2020

@NinoFloris @TheAngryByrd Have addressed the feedback

@TheAngryByrd
Copy link
Collaborator

Thank you!

@TheAngryByrd TheAngryByrd merged commit 275e465 into demystifyfp:master Nov 18, 2020
TheAngryByrd added a commit that referenced this pull request Nov 18, 2020
- Switches TaskResult Library from TaskBuilder to Ply. Credits [Nino Floris](https://github.com/NinoFloris) - (#97)
- This change replaces [TaskBuilder](https://github.com/rspeele/TaskBuilder.fs) with [Ply](https://github.com/crowded/ply).  Ply has better performance characteristics and more in line with how C# handles Task execution.  To convert from TaskBuilder to Ply, replace the namespace of `FSharp.Control.Tasks.V2.ContextInsensitive` with `FSharp.Control.Tasks.NonAffine`. -
- This also removes the TargetFramework net461 as a build target. Current netstandard2.0 supports net472 fully according to [this chart](https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). It's recommended to upgrade your application to net472 if possible. If not, older versions of this library, such as 1.4.3, aren't going anywhere and can still be consumed from older TargetFrameworks.
- Switch to use Affine for Task related. Credits [@Swoorup](https://github.com/Swoorup). - (#107)
@TheAngryByrd
Copy link
Collaborator

@Swoorup Take 2.0.0-beta002 for a spin

@Swoorup Swoorup deleted the sj-affine-tasks branch November 18, 2020 23:34
@Swoorup
Copy link
Contributor Author

Swoorup commented Nov 19, 2020

Pre 2.0.0-beta002, the following code would give

#r "nuget: Ply"
#r "nuget: FsToolkit.ErrorHandling.TaskResult,2.0.0-beta002"

open System
open System.Threading
open System.Threading.Tasks
open FSharp.Control.Tasks
open FsToolkit.ErrorHandling
open FsToolkit.ErrorHandling

let a () =
  task {
    printfn "From taskCE Scheduler Id: %A" TaskScheduler.Current.Id
    let! a = 
      taskResult { 
        do! Task.Delay(TimeSpan.FromSeconds(1.0)) 
        printfn "From taskResultCE Scheduler Id: %A" TaskScheduler.Current.Id
      }
    return ()
  } 

let Scheduler = ConcurrentExclusiveSchedulerPair(TaskScheduler.Default, 1, 1)
printfn "Main Scheduler ID: %A" TaskScheduler.Current.Id
printfn "Scheduler ID: %A" Scheduler.ExclusiveScheduler.Id

Task.Factory.StartNew(a, CancellationToken.None, TaskCreationOptions.None, Scheduler.ExclusiveScheduler) |> ignore
Console.ReadLine()

Pre 2.0.0-beta002

image

2.0.0-beta002

image

Works well now.

TheAngryByrd added a commit that referenced this pull request Nov 20, 2020
- Switches TaskResult Library from TaskBuilder to Ply. Credits [Nino Floris](https://github.com/NinoFloris) - (#97)
- This change replaces [TaskBuilder](https://github.com/rspeele/TaskBuilder.fs) with [Ply](https://github.com/crowded/ply).  Ply has better performance characteristics and more in line with how C# handles Task execution.  To convert from TaskBuilder to Ply, replace the namespace of `FSharp.Control.Tasks.V2.ContextInsensitive` with `FSharp.Control.Tasks.NonAffine`. -
- This also removes the TargetFramework net461 as a build target. Current netstandard2.0 supports net472 fully according to [this chart](https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). It's recommended to upgrade your application to net472 if possible. If not, older versions of this library, such as 1.4.3, aren't going anywhere and can still be consumed from older TargetFrameworks.
- Switch to use Affine for Task related. Credits [@Swoorup](https://github.com/Swoorup). - (#107)
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

Successfully merging this pull request may close these issues.

Use affine tasks instead of non-affine
3 participants