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

Task ate my data! Unexpected results at runtime. No errors in core stack. #13404

Closed
simontreanor opened this issue Jun 28, 2022 · 14 comments
Closed
Labels
Area-Compiler-StateMachines Sequence, list, task and other state machine compilation Bug
Milestone

Comments

@simontreanor
Copy link

Description of the issue

Converting an existing async function to the new F# 6 task CE throws up some unexpected and undesirable behaviour:

  • Runtime failure, not compiler failure
  • No errors thrown in core stack at all, just lost data

Steps required to reproduce the problem:

The attached ZIP file contains two .NET Interactive Notebooks, test_async.dib, which is the original function using async, and test_task.dib, which uses the new task CE. Running them yields the results described. The data array gets swallowed up.

test.dib.zip

Other notes:

  • Weird error if you remove unused fromAccount variable declaration:
    System.InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed

Expected result

True, [ ( 2022-06-28 18:06:32Z, 3, 4, 5, { Some(6): Value: 6 }, { Some(1): Value: 1 }, True, ( { Some(7): Value: 7 }, True, 2022-06-28 18:06:32Z ) ), ( 2022-06-28 18:06:32Z, 3, 4, 5, { Some(6): Value: 6 }, { Some(1): Value: 1 }, True, ( { Some(7): Value: 7 }, True, 2022-06-28 18:06:32Z ) ), ( 2022-06-28 18:06:32Z, 3, 4, 5, { Some(6): Value: 6 }, { Some(1): Value: 1 }, True, ( { Some(7): Value: 7 }, True, 2022-06-28 18:06:32Z ) ) ], 3

Actual result

True, null, 3

Known workarounds

  1. Change task to async -> Works, but reverts to slower workflow. The code has no real async operations.
  2. Move mapPrioritiesTransfers tasks inside match conditions -> Works, but high overhead to convert code
  3. remove minOrder and set isPrio to true -> Works, but altered behaviour

There is no clear understanding of when we need a workaround for what, leading to a lack of ability to trust the new task-construct, so it seems rather fragile.

Related information

  • Win 11
  • .NET SDK 6
  • F# 6.0
  • VS2022 and VSCode
@Thorium
Copy link
Contributor

Thorium commented Jun 28, 2022

If I create a new console app and add paste the code there (with open System and printfn "%A" test.Result) the observations:

  • Happens on Release mode only, not on Debug. Happens systematically, not random.
  • Seems to happen on both .NET6 (Core) and .Net Framework 4.7.2
  • One more random non-optimal workaround: Seems to happen on Arrays but if you replace all the instances to Seq or List, it seems to work better. I have no idea why, and it's probably not good advice to do performant Task code and try to avoid Arrays. Probably because then it's not going to "alternate dynamic implementation" of task.
  • If the dynamic is this dangerous, then "warning FS3511: This state machine is not statically compilable..." should be an error, not a warning. How does a developer know what mode is used? It shouldn't matter the developer, right?

Could it be that if you have multiple taskbuilders, or the task continuation changes the execution thread, somehow something here is not thread-safe or overwrites the state of another task?

tasks.fs type TaskBuilder, RunDynamic(code: TaskCode<'T, 'T>) what is this sm.ResumptionDynamicInfo.ResumptionData <- null ?

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

@simontreanor

  1. What exact version of .NET notebooks do you have please?
  2. This bug looks highly relevant, as it is related to uses of Array.map in tasks System.NullReferenceException in new task CE in F# 6 #12359, it has been fixed but it's possible the fix is not yet in .NET Notebooks.
  3. Could you try replacing Array.map and perhaps any other inlined Array functions with your own non-inlined versions

@Thorium What's your exact VS or VSCode or .NET SDK version please

BTW I'd imagine a fix is to move the chunks of synchronous code out of task { ... }

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

@Thorium reports that he's using latest SDK which I assume means 6.0.301 or later

I can confirm that a workaround is to move synchronous code using Array.map out of the task { ... }. You can also workaround by using this at the top of your file:

module Array =
    [<MethodImpl(MethodImplOptions.NoInlining)>]
    let map f xs = FSharp.Collections.Array.map f xs

Though you may want to use this code instead, so you can scan for Array.map and force the use of Array.mapFixed:

module Array =
    [<MethodImpl(MethodImplOptions.NoInlining)>]
    let mapFixed f xs = FSharp.Collections.Array.map f xs

    [<Obsolete("Do not use Array.map in this project because of https://github.com/dotnet/fsharp/issues/13404", true)>]
    let map f xs = FSharp.Collections.Array.map f xs

@isaacabraham
Copy link
Contributor

So is this actually a bug or just that it's using an early version of F#6 which had that issue with Array.map (which was my first thought when seeing this) - has it been reproed in e.g. FSI directly or VS/VSCode etc.?

@simontreanor
Copy link
Author

simontreanor commented Jun 29, 2022 via email

@vzarytovskii
Copy link
Member

vzarytovskii commented Jun 29, 2022

I'm on the latest version of everything.

If it's a notebook, the kernel can be using an old FSharp.Compiler.Service package.

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

@isaacabraham This certainly looks like another variation of the bug of #12359, unfixed as yet. Also #12337 may be relevant.

The circumstances appear to be:

  • A single task has multiple Array.map or other functions that are inlined to imperative loops
  • These loops manipulate a struct type like Guid
  • You're using Release code
  • You get the warning warning FS3511: This state machine is not statically compilable. A resumable code invocation at '(50,12--50,16)' could not be reduced. An alternative dynamic implementation will be used, which may be slower. Consider adjusting your code to ensure this state machine is statically compilable, or else suppress this warning.

The underlying problem is that Array.map is inlined to an imperative while loop. This appears to be interacting with logic for zero-initialization of locals of struct type like Guid. From playing around it seems the problem only occurs when there are multiple inlined loops working on type Guid - indeed even more specific than that. It's quite tricky to get a really small repro though we will work on it.

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

A smaller repro that causes a different exception is at https://gist.github.com/dsyme/0e748fbab29e0ba4ca15cfefdf88055b

dotnet fsi --optimize+ c:\misc\bug.fsx

or in repo:

artifacts\bin\fsi\Debug\net6.0\fsi.exe --optimize+ c:\misc\bug.fsx

I haven't yet found a repro that doesn't involve FS3511 being generated. Please let me know if you do. I also haven't found a repro that doesn't involve at least three Array.map and nested tasks

@isaacabraham
Copy link
Contributor

@vzarytovskii thanks, that's what I was really getting at hence my suggestion re: FSI / VS etc. :-)

@Thorium
Copy link
Contributor

Thorium commented Jun 29, 2022

I'm on the latest version of everything.

If it's a notebook, the kernel can be using an old FSharp.Compiler.Service package.

notebook is just a detail to reproduce the issue, this happens also new non-notebook.

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

Smallish repro that gives a peverify failure: https://gist.github.com/dsyme/9523e4738707311dd5bc4b3933689f32. I have a fix for this. Not sure if it fixes the whole problem

@dsyme
Copy link
Contributor

dsyme commented Jun 29, 2022

OK I have this fixed - at least the data corruption part (caused by incorrect codegen).

The bug will occur when a nested task fails state machine compilation. A realiable workaround is if a nested task gives FS3511 you should move it out to a separate function.

The failure of state machine compilation is separate issue.

@Thorium
Copy link
Contributor

Thorium commented Jul 4, 2022

That's excellent, thanks.
I hope the state machine will get fixed at some point too.
Getting "warning FS3511: This state machine is not statically compilable." has a feeling that I have to please the compiler, not like the typical FSharp where the compiler is there to help me.

@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Jul 7, 2022
@vzarytovskii vzarytovskii added this to the Backlog milestone Jul 7, 2022
TheAngryByrd added a commit to demetrixbio/Plough.WebApi that referenced this issue Jul 21, 2022
- After experiencing [still many bugs with native tasks](dotnet/fsharp#13404) we're gonna revert the changes (again) to use Ply.
- This also upgrades the build infrastructure to make releasing easier
@dsyme dsyme added the Area-Compiler-StateMachines Sequence, list, task and other state machine compilation label Aug 16, 2022
@dsyme
Copy link
Contributor

dsyme commented Aug 16, 2022

The lack of static complation of some tasks is being tracked here: #12839

@dsyme dsyme closed this as completed Aug 16, 2022
Repository owner moved this from Not Planned to Done in F# Compiler and Tooling Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-StateMachines Sequence, list, task and other state machine compilation Bug
Projects
Archived in project
Development

No branches or pull requests

5 participants