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

Add F# and VB.NET samples #2046

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Conversation

martincostello
Copy link
Member

@martincostello martincostello commented Apr 9, 2024

Issue #2045 pointed out that the usage for Polly v8 isn't as terse as it is for C# due to the inability to directly await ValueTask{<T>}.

This PR adds some limited F# and VB.NET examples to the documentation that demonstrate how to bridge the gap between ValueTask and Task for Polly v8 pipelines.

I also wonder if in a separate PR in a future release, we could maybe introduce some extension methods to make interop with Task easier as an opt-in at the expense of allocating more?

Something like this:

public static async Task ExecuteAsTaskAsync<TState>(
    this ResiliencePipeline pipeline,
    Func<ResilienceContext, TState, Task> callback,
    ResilienceContext context,
    TState state)
{
    var converted = async (context, state) =>
    {
        await callback(context, state);
        return ValueTask.Completed;
    };
    return pipeline.ExecuteAsync(converted, context, state).AsTask();
}

Then it could be consumed like this without needing to use AsTask() or return a ValueTask from the user's callback:

Dim result = Await pipeline.ExecuteAsTaskAsync(Async Function(state, context)
                                                   await Task.Delay(1000)
                                               End Function, _
                                               context, _
                                               state)

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.67%. Comparing base (4051e2a) to head (fbbc057).
Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2046   +/-   ##
=======================================
  Coverage   83.67%   83.67%           
=======================================
  Files         312      312           
  Lines        7105     7106    +1     
  Branches     1054     1054           
=======================================
+ Hits         5945     5946    +1     
  Misses        789      789           
  Partials      371      371           
Flag Coverage Δ
linux 83.67% <ø> (+<0.01%) ⬆️
macos ?
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add documentation for using Polly with F# and VB.
Add some additional terminology.
@martincostello
Copy link
Member Author

Thoughts @martintmk and @peter-csala?

Make the sample for using Polly from F# more idiomatic for those users.

Co-Authored-By: Stuart Lang <stuart.b.lang@gmail.com>
@peter-csala
Copy link
Contributor

peter-csala commented Apr 10, 2024

Thoughts @martintmk and @peter-csala?

I'm not a VB or F# expert but I think we should either add direct support to these languages or say that they are not supported. We can't expect that everyone will read our samples and apply the AsTask workaround everywhere.

I'm not even sure that this solution would work for more complex scenarios as well. The sample codes are too simple to make a fair judgement whether this workaround is applicable everywhere.

@martincostello
Copy link
Member Author

not supported

I wouldn't say it's not supported, it's just difficult. We did however neglect to consider the impact on VB.NET and F#, which is our bad, but given we're not familiar with those languages in the first place, I don't think it's a failure on our part.

The sample codes are too simple to make a fair judgement whether this workaround is applicable everywhere.

I asked @slang25 who knows F# well to review my initial sample, and it seems to be fairly trivial to make it work compared to VB (adding |> ValueTask).

I intentionally left them simple so that someone who considers themselves an "expert" can make their own judgement on whether they want to use it that way or not if they read the documentation. A VB guru might just say to themselves "this is too hard" and not use it, which is fine, if unfortunate. A "novice" might decide they're fine if they need to use it in one place and haven't ever used Polly before. The "this is too hard" outcome is what the original user who brought this to my attention went with.

As noted in the PR comment, we could consider adding some extension methods that do the Task => ValueTask => Task wrapping for the user at the expense of the additional allocations/overhead that entails.

I don't think there's otherwise anything we can do about it other than accept it's not a good experience in those languages and that maybe one day F# will catch up to C# in terms of native ValueTask await support. Going back to Task would undo the main point of why Polly v8 was created in the first place, as well as be yet another breaking change.

@martintmk
Copy link
Contributor

@martincostello

I am worried tha adding these extensions might introduce ambiguity. If you remember, at some point, we actually had these but decided to drop them in favor of having ValueTask based API only.

Since the VB and F# is not our main scenario. how about we make the life to these consumers easier by introducing these extensions in something like Polly.Interop? This would make the core API surface small and clean.

@martincostello
Copy link
Member Author

Yeah, I was thinking about putting them in a new package rather than in Core. Was more the API shape idea I was looking for feedback on initially.

@peter-csala
Copy link
Contributor

Yeah, I was thinking about putting them in a new package rather than in Core. Was more the API shape idea I was looking for feedback on initially.

Does it impact only the ExecuteAsync and ExecuteOutcomeAsync overloads or anything where the API expects a ValueTask like ShouldHandle, DelayGenerator or OnRetry?

@martincostello
Copy link
Member Author

martincostello commented Apr 10, 2024

Any method member returning ValueTask or ValueTask<T> - VB and F# can't await anything other than Task or Task<T>.

I'm not suggesting we augment every single API - just the core/basic use cases.

@peter-csala
Copy link
Contributor

peter-csala commented Apr 10, 2024

Any method returning ValueTask or ValueTask<T> - VB and F# can't await anything other than Task or Task<T>.

I'm not suggesting we augment every single API - just the core/basic use cases.

All the above listed properties are methods that are returning either ValueTask or ValueTask<T>.

Because of that, I can do this:

Dim options As New TimeoutStrategyOptions With {.Timeout = TimeSpan.FromSeconds(1), 
	.OnTimeout = Function(args)
		Console.WriteLine("Timed out")
		Return New ValueTask()
     End Function}

but I can't do this

Dim options As New TimeoutStrategyOptions With {.Timeout = TimeSpan.FromSeconds(1), 
	.OnTimeout = Async Function(args)
		Await Task.Delay(1000)
		Console.WriteLine("Timed out")
     End Function}

The 'Async' modifier can only be used on Subs, or on Functions that return Task or Task(Of T).

@baronfel
Copy link
Contributor

baronfel commented Apr 15, 2024

Out of the box F# cannot create ValueTasks with nice syntax, but it can consume them just fine. For creation, there are a few ways to get to something usable. Here's a quick sample showing my two preferred ways:

module Polly.FSharp

open Polly
open System
open System.Threading
open System.Threading.Tasks

let pipeline =
    ResiliencePipelineBuilder()
        .AddRetry(Retry.RetryStrategyOptions())
        .AddTimeout(TimeSpan.FromSeconds 5.)
        .Build()

let token = CancellationToken.None

// this creates a Task using the F# built-in Task builder, then creates a valueTask from it
// this is more allocate-y but works out of the box. The default 'task' builder ends up returning a Task<int> at the top-level
let builtinValue =
    task {
        let! result = pipeline.ExecuteAsync((fun token -> (task { return 42 }) |> ValueTask<int>), token)
        return result
    }


// this uses the valueTask builder from IcedTasks to have 'direct' use of ValueTasks
// lower allocations, but requires adding a library (though one that is pretty widely used in the F# community).
// this version ends up returning a ValueTask<int> at the top-level.
// `dotnet add package IcedTasks`
open IcedTasks

let vTaskResult =
    valueTask {
        let! result = pipeline.ExecuteAsync((fun token -> valueTask { return 42 }), token)
        return result
    }

cc @TheAngryByrd

I also wanted to show the difference in the 'outer' experience for the user. Here's a screenshot from my editor to illustrate:

image

With the 'native' solution, you are working with Task<'t> externally - the task CE can await ValueTask's (as you might expect), but there's no built-in way to create ValueTasks. Then, with IcedTask's valueTask builder you can stay ValueTask both on the consumption side as well as the creation side.

@martincostello
Copy link
Member Author

Let's move the discussion of any possible new APIs to #2061.

I'll look to apply some further feedback to the F# sample based on @baronfel's suggestions so we can look to get this merged to provide some help to future C#/VB users trying to adopt Polly.

Apply feedback to use IcedTasks to consume ValueTask in the C# sample.

Co-Authored-By: Chet Husk <573979+baronfel@users.noreply.github.com>
@martincostello
Copy link
Member Author

@baronfel @TheAngryByrd I've updated the F# sample to use IcedTasks as suggested - let me know if there's anything further that can be tightened up.

Copy link
Contributor

@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 from F# point of view :)

let getBestFilmAsync token =
task {
do! Task.Delay(1000, token)
return "https://www.imdb.com/title/tt0080684/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Good choice :)

Copy link
Contributor

@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.

Oops after I comment I found something. This has an extra valueTask wrapper in places that doesn't need to be here.

Comment on lines 32 to 43
do!
valueTask {
do! pipeline.ExecuteAsync(
fun token ->
valueTask {
printfn "Hello, world! Waiting for 2 seconds..."
do! Task.Delay(1000, token)
printfn "Wait complete."
}
, token
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops after I comment I found something. This has an extra valueTask wrapper that doesn't need to be here.

Suggested change
do!
valueTask {
do! pipeline.ExecuteAsync(
fun token ->
valueTask {
printfn "Hello, world! Waiting for 2 seconds..."
do! Task.Delay(1000, token)
printfn "Wait complete."
}
, token
)
}
do! pipeline.ExecuteAsync(
fun token ->
valueTask {
printfn "Hello, world! Waiting for 2 seconds..."
do! Task.Delay(1000, token)
printfn "Wait complete."
}
, token
)

Comment on lines 51 to 58
let! bestFilm =
valueTask {
let! url = pipeline.ExecuteAsync((fun token -> valueTask {
let! url = getBestFilmAsync(token)
return url
}), token)
return url
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let! bestFilm =
valueTask {
let! url = pipeline.ExecuteAsync((fun token -> valueTask {
let! url = getBestFilmAsync(token)
return url
}), token)
return url
}
let! bestFilm =
pipeline.ExecuteAsync((fun token -> valueTask {
let! url = getBestFilmAsync(token)
return url
}), token)

Remove `valueTask` wrappers that are redundant.

Co-Authored-By: Jimmy Byrd <1490044+theangrybyrd@users.noreply.github.com>
@martincostello martincostello marked this pull request as ready for review April 18, 2024 14:27
@martincostello martincostello enabled auto-merge (squash) April 18, 2024 14:28
@martincostello martincostello merged commit 39cc454 into App-vNext:main Apr 18, 2024
15 checks passed
@martincostello martincostello deleted the fs-vb-samples branch April 18, 2024 15:03
@martincostello
Copy link
Member Author

Thanks for the feedback everyone. The docs are now live here: https://www.pollydocs.org/use-with-fsharp-and-visual-basic.html

@martincostello martincostello mentioned this pull request Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Question]: How to use ExecuteAsync In VB.Net?
5 participants