WIP: Run final targets after user cancels with ctrl+c #1946

kblohm · 2018-05-20T22:01:47Z

Hi,
i am not sure if this is needed/wanted but i wanted to try it out ;).
When the user presses ctrl+c, all running tasks are cancelled and all already started processes are killed. Afterwards the finalTargets are run, unless the user presses ctrl+c again to forcefully cancel.
I am also not sure if this is the right approach, but maybe you can have a look.
Some of the changes are just so that i can #load the target-module and test stuff.

matthid · 2018-05-20T22:14:39Z

Clever, I had a similar idea in mind (just killing all processes). The only thing this doesn't handle is when the target function itself is still doing something (and not waiting for external functions) or if the target is just running the next process after we killed the previous one (some tasks have an internal retry...).

In fact, I feel like in practice the solution you propose is already more than useful - maybe we can kind of fall-back to our current behavior when the user presses ctrl+c a second time (or the shell might be killing us anyway in that case). Good thing is that the token is available via the context, so people can "fix"/adapt their target code if needed.

kblohm · 2018-05-20T22:21:32Z

What is the current behavior? Istn that just letting the shell kill the process? Or is there already some cleanup-code i did not manage to find?

matthid · 2018-05-20T22:26:24Z

No I think the shell is just killing stuff. However if msbuild is running it prints "attempting to kill..." or something like that so the signal already seems to flow though fake (but on the other side I have no idea how things work behind the scenes)

matthid

Looks very very reasonable. Some suggestions/question in-line

matthid · 2018-05-21T14:54:21Z

src/app/Fake.Core.Target/Target.fs

+                    f a)
+
+            let cancelHandler  = captureContext (handleUserCancelEvent cts)            
+            use __ = Console.CancelKeyPress |> Observable.subscribeOnce cancelHandler


Does this throw in non-interactive mode (CI systems?) or just do nothing?

I dont know. As no one is pressing any keys there, that should just work. Do you have any idea on how to test that?

We can keep this in mind and test on staging

matthid · 2018-05-21T14:56:44Z

src/app/Fake.Core.Target/Target.fs

@@ -584,15 +596,16 @@ module Target =
                member __.GetNextTarget (ctx) = mbox.PostAndAsyncReply(fun reply -> GetNextTarget(ctx, reply))
            }

-        let runOptimal workerNum (order:Target[] list) targetContext =
+        let runOptimal workerNum (order:Target[] list) targetContext=


add that space again :)

matthid · 2018-05-21T14:57:15Z

src/app/Fake.Core.Target/Target.fs

@@ -159,10 +167,12 @@ module Target =
                Trace.traceError  <| sprintf "  - %s" target.Value.Name
            failwithf "Target \"%s\" is not defined." name

-    let internal runSimpleInternal context target =


matthid · 2018-05-21T14:58:56Z

src/app/Fake.Core.Target/Target.fs

+      Arguments : string list
+      IsRunningFinalTargets : bool
+      CancellationToken : CancellationToken }
+    static member Create ft all args isRunningFinalTargets (token:CancellationToken)= { 


Just start with IsRunningFinalTargets = false? (also spacing around =)

I need a formatting tool ;). you mean IsRunningFinalTargets as the first argument? Is the bool OK or should that be an enum with all the possible states?

bool is okish, but Create doesn't need it as argument is what I suggest

matthid · 2018-05-21T15:00:56Z

src/app/Fake.Core.Target/Target.fs

+                    member __.OnCompleted()=                           
+                        remove()
+                }
+            observable.Subscribe observer 


So this is basically observable.Take(1), correct?

Yes, should be Take(1)/First. But i did not want to add an extra library

I think this has a race when the OnNext event is triggered before setRemover is called

Trying to find a suggestion ...

Isnt this pretty much the same as here ?

Yes and in fact I feel like it has the same problem. It probably will just work for our use case.

https://github.com/fsprojects/FSharp.Control.Reactive supports netstandard2.0 now.

Yes actually I have no problem with adding that dependency as I'm not able to come up with a solution and give up :)

How important is .netstandard1.6 support? I would have to remove that.

I'm pretty sure we can live without netstandard1.6

matthid · 2018-05-21T15:02:43Z

src/app/Fake.Core.Target/Target.fs

+                    // always try to keep "parallelJobs" runners busy
+                    ParallelRunner.runOptimal parallelJobs order context
+                    //order
+                    //    |> Seq.fold (fun context par -> runTargetsParallel parallelJobs par context) context


I think we can remove this now as well as the implementation if it still exists (though I can see if you don't want to do it as part of this PR - I wouldn't mind if we already do changes here)

(Take this with caution:) Maybe we even can unify this now with the other branch (though I'm not sure if that actually looks better)

You mean remove the ParallelRunner.runOptimal method and the whole if parallelJobs > 1 && not singleTarget ?

That would then remove the whole ParallelRunner as far as i can see.

No, I meant the comments and runTargetsParallel

OK, i can remove those. That confused me ;)

matthid · 2018-05-21T15:06:05Z

src/test/Fake.Core.UnitTests/Fake.Core.Target.fs

+      let mutable finalTargetResult = 0
+      Target.createFinal "Final" (fun _ -> finalTargetResult <- 1)
+
+      let context = run "c"


Maybe we should let some tests run with a parallel version of run

I was actually going to add tests for the cancellation, but i am not sure how to do that. It would probaby have to run in another Process? And since i did not really manage to do that i at least added some tests for the normal final-targets :/.
CoreFx has some for cancellation here. But that is quite a lot of boilerblate to add.

I think cancellation is fine, I meant is just letting it run through the parallel code paths and see if the result still is fine (yes in fact that might mean that stuff is our of order if dependencies allow it but we can still do basic checks)

matthid · 2018-05-21T15:10:30Z

src/test/Fake.Core.UnitTests/Fake.Core.Target.fs

+      let mutable failureTargetResult = 0
+      Target.createBuildFailure "FailureTarget" (fun _ -> failureTargetResult <- 1)
+      Target.createBuildFailure "FailureTarget2" (fun _ -> failureTargetResult <- failureTargetResult+1)
+      Target.activateBuildFailure "FailureTarget"


As far as I understand we have to activate manually because people use this as part of other targets (it they activate FailureTarget as part of target b for example.

In an ideal world I'm thinking about doing that over the context and not via global mutable state - and in fact "freeze" the state once "run" is called. However I don't think we can do this at this point without breaking change.
Sadly that one slipped through when re-designing the API... Something to keep in mind for fake 6

Do we need tests then to cover activation in another target? That would also only work at specific timings, so yeah that looks kinda strange.

kblohm · 2018-05-21T17:57:05Z

src/app/Fake.Core.Target/Target.fs

            let mgr = createCtxMgr order targetContext
            let targetRunner () =
                async {
+                    let token = targetContext.CancellationToken


Instead of using the token from the context here, should i add it to Async.StartAsTask and Async.RunSynchronously ? I guess i would have to add it to both because of the Task.AwaitAll?

Actually I think it is fine as it is, using the default cancellation token has funny side effects. We want the runner to stay alive in order to execute the final targets.

matthid · 2018-05-21T18:41:24Z

I'll merge this and some some tests for the parallel branch. Or are you still working on something right now?

kblohm · 2018-05-21T18:44:28Z

yes, i am fixing all the stuff we discussed

kblohm · 2018-05-21T18:45:31Z

I am running the build locally, if that passes i will commit. Do you want a new commit so you can see the changes easier? Otherwise i will ammend.

matthid · 2018-05-21T18:48:45Z

As you like ;) More commits lead to more contribution points on github :P

kblohm · 2018-05-21T18:54:02Z

Everything for internetpoints!

kblohm · 2018-05-21T19:00:29Z

Things we could still do:

Kill running processes when aborting final targets
Kill process-tree (i dont think Process.killAllCreatedProcesses() kills childprocesses)
Handle ctrl+c or at least killing of processes in runSimple

matthid · 2018-05-21T19:11:48Z

Ok let's see, nice stuff!

kblohm · 2018-05-21T19:54:40Z

As a side note for the tests with expecto: have you looked at https://github.com/YoloDev/YoloDev.Expecto.TestSdk ? No idea how good that is, but some people seem to be using it for dotnet core.

matthid · 2018-05-21T23:09:33Z

Relevant to your process-tree comment:

https://github.com/dotnet/cli/issues/7426

kblohm · 2018-05-22T17:54:23Z

This is what some of the dotnet people are using "solve" that.

matthid · 2018-05-22T17:56:46Z

@kblohm Looks interesting, we might want to "save" that in a new issue. But as long as we have no reports about it ;)

0x53A · 2018-05-22T19:51:05Z

But as long as we have no reports about it ;)

cough #1427

This would be a nice feature to add and a PR is very welcome. Closing this for now.

But yeah, I never did send a PR

0x53A · 2018-05-22T19:54:25Z

Theoretically, if someone might want to start on this, what would the design be?

kill all trees
only kill trees with opt-in from user (would require adding a parameter to everything that spawns a process)
kill everything, except opt-out from user, would also require adding this parameter

matthid · 2018-05-22T21:26:15Z

Yes or we just change the default behavior and don't make it opt-out either. Actually the process killing is already opt-out afaik

matthid · 2018-05-22T21:26:41Z

As in the old issue: PR is welcome ;)

0x53A · 2018-05-22T21:27:31Z

so 1)? Kill all children started by FAKE?

matthid · 2018-05-22T21:35:09Z

Yes I don't see how that is worse than killing everything (ie what we already do)

kblohm force-pushed the handleUserCancel branch from 2714e29 to c36795f Compare May 20, 2018 22:48

Run final targets after user cancels with ctrl+c

c1c5acf

kblohm force-pushed the handleUserCancel branch from c36795f to c1c5acf Compare May 21, 2018 12:33

matthid approved these changes May 21, 2018

View reviewed changes

kblohm commented May 21, 2018

View reviewed changes

Use FSharp.Control.Reactive and some Cleanup

e3242b1

matthid changed the base branch from master to release/rc May 21, 2018 18:57

matthid merged commit 4ee2ee3 into fsprojects:release/rc May 21, 2018

kblohm deleted the handleUserCancel branch May 21, 2018 19:52

matthid mentioned this pull request May 21, 2018

Update targets to allow deactivation #1928

Merged

WIP: Run final targets after user cancels with ctrl+c #1946

WIP: Run final targets after user cancels with ctrl+c #1946

Conversation

kblohm commented May 20, 2018

matthid commented May 20, 2018

kblohm commented May 20, 2018

matthid commented May 20, 2018

matthid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kblohm May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kblohm May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented May 21, 2018

kblohm commented May 21, 2018

kblohm commented May 21, 2018

matthid commented May 21, 2018

kblohm commented May 21, 2018

kblohm commented May 21, 2018

matthid commented May 21, 2018

kblohm commented May 21, 2018

matthid commented May 21, 2018

kblohm commented May 22, 2018

matthid commented May 22, 2018

0x53A commented May 22, 2018

0x53A commented May 22, 2018

matthid commented May 22, 2018

matthid commented May 22, 2018

0x53A commented May 22, 2018

matthid commented May 22, 2018

kblohm May 21, 2018 •

edited

Loading

matthid May 21, 2018 •

edited

Loading

kblohm May 21, 2018 •

edited

Loading