Adding cancel execution command #1057

colombod · 2021-02-10T23:57:21Z

Add tests for quit command

src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

jonsequitur · 2021-02-16T17:15:11Z

src/Microsoft.DotNet.Interactive.Tests/CompositeKernelTests.cs

        }

+        [Fact]
+        public async Task quit_command_cancels_all_deferred_commands_on_composite_kernel()


These aren't really just tests of the CompositeKernel. It might be useful to split them into a new test class and move toward a more vertical test organization.

moved to QuiCommandTests class

jonsequitur · 2021-02-16T17:19:54Z

src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

+                    {
+                        await Task.Delay(100);
+                        await kernel.SendAsync(cancelCommand);
+                    })).Wait();


Make sure to enforce a timeout here.

jonsequitur · 2021-02-16T17:20:43Z

src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

+        [Theory]
+        [InlineData(Language.CSharp)]
+        [InlineData(Language.FSharp)]
+        public async Task cancel_command_cancels_all_deferred_commands(Language language)


This looks like a duplicate. Consolidating will help make it clearer what we're testing.

jonsequitur · 2021-02-16T17:21:58Z

src/Microsoft.DotNet.Interactive/Commands/Cancel.cs

+    {
+        public Cancel(string targetKernelName = null): base(targetKernelName)
+        {
+            Handler = (command, context) => Task.CompletedTask;


It's preferable to override InvokeAsync. We should at some point limit the settable Handler property to only those commands that vary behavior by kernel.

jonsequitur · 2021-02-16T17:23:13Z

src/Microsoft.DotNet.Interactive/CompositeKernel.cs

+                    ClearPendingCommands();
+                    kernel.CancelInflightCommands();
+                    kernel.ClearPendingCommands();
+                    break;


Do these two cases need to vary in terms of behavior?

might not be the case

jonsequitur · 2021-02-16T17:23:34Z

src/Microsoft.DotNet.Interactive/CompositeKernel.cs

+                case Cancel _:
+
+                    CancelInflightCommands();
+                    ClearPendingCommands();


Is "pending" different from "deferred"?

pending includes all things in the trampoline queue and the deferred

So it might be clearer to say ClearQueuedAndDeferredCommand since both of those are terms that already exist in the code.

jonsequitur · 2021-02-16T17:25:17Z

src/Microsoft.DotNet.Interactive/CompositeKernel.cs

                throw new NoSuitableKernelException(command);
            }

+            switch (command)


Rather than duplicating all of this, why not move it to the base and put the actual cancellation logic into a virtual method?

jonsequitur · 2021-02-16T17:28:50Z

src/Microsoft.DotNet.Interactive/Commands/Quit.cs

+    {
+        private static readonly CompositeDisposable DisposeOnQuit = new();
+
+        public static void RegisterForDisposalOnQuit(IDisposable disposable)


Why have potentially different disposal semantics than Kernel.Dispose? This looks like something that doesn't belong in the library. It should probably just be a host concern.

we can have the host to register the OnQuit handler, and throw if it is not set

jonsequitur · 2021-02-16T17:29:12Z

src/Microsoft.DotNet.Interactive/Commands/Quit.cs

+
+        public Quit(Action onQuit, string targetKernelName = null) : base(targetKernelName)
+        {
+            if (onQuit == null)


Suggested change

if (onQuit == null)

if (onQuit is null)

jonsequitur · 2021-02-16T17:31:02Z

src/Microsoft.DotNet.Interactive/KernelInvocationContext.cs

-            _events.OnCompleted();
-            IsComplete = true;
+                _events.OnCompleted();
+                _cancellationTokenSource.Cancel(false);


It looks like this gets called twice when someone is calling Cancel.

jonsequitur · 2021-02-17T20:38:28Z

src/Microsoft.DotNet.Interactive.Tests/QuitCommandTests.cs

+
+namespace Microsoft.DotNet.Interactive.Tests
+{
+    [Collection("Do not parallelize")]


Why should these not be parallelizable? That sounds like a smell.

remove unused methods

deferred command that match the kernel used during scheduling of a command are executed deferred command order is preserved and parent kernel deferred commands are executed when scheduling commands on child kernel do not produce diagnostic events when there are none use callable command

exception handling for code in scheduled work remove blank lines

colombod requested a review from jonsequitur February 10, 2021 23:57

runfoapp bot mentioned this pull request Feb 11, 2021

OSX deprovision jaredpar/runfo#41

Closed

colombod requested a review from brettfo February 11, 2021 20:02

colombod force-pushed the cancel_execution_command branch 3 times, most recently from 76f33f9 to 2083ddf Compare February 15, 2021 01:05

jonsequitur reviewed Feb 16, 2021

View reviewed changes

src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Feb 16, 2021

View reviewed changes

colombod force-pushed the cancel_execution_command branch 5 times, most recently from 8a2ce09 to fba0b52 Compare February 17, 2021 20:18

jonsequitur reviewed Feb 17, 2021

View reviewed changes

colombod force-pushed the cancel_execution_command branch 7 times, most recently from 5289339 to e3aa6bd Compare February 20, 2021 00:24

colombod added 20 commits February 23, 2021 22:38

reflect changes in contract.ts

a282d76

refactor cancelCommands

2bcc44e

remove unused methods

avoid double cancellation request

b27cc5f

add test to ensure commands are runing after cancel is issued

47a35f3

introduce KernelCommandScheduler

c43018f

experiment

16d0d55

new scheduler api

d32c122

scheduled work must be awaited before completing the source

37e9b31

cleanup

7368e0d

cancel work on scheduler

50a9bff

refactoring

44713b3

refactor test

c62ff9b

disable warning for fire and forget

87de3ea

removed pragma

99b10e1

disposable scheduler disposed during tests

8edf855

test parallel execution

885ab81

test for parallelism

a8c5b86

test

2c470e9

exception handling for code in scheduled work remove blank lines

remove namespace

314e740

colombod force-pushed the cancel_execution_command branch from ab93a75 to 314e740 Compare February 23, 2021 22:39

colombod added 3 commits February 23, 2021 23:08

change api surface

d7c3353

remove extension methods

682d49c

all broken here

88cc568

colombod force-pushed the cancel_execution_command branch from d05b3d6 to 88cc568 Compare February 24, 2021 00:54

colombod closed this Feb 25, 2021

colombod deleted the cancel_execution_command branch February 26, 2021 00:57

This was referenced Jan 18, 2022

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.1.1 aliscco/interactive#17

Open

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.1.1 aliscco/interactive#18

Open

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.1.1 aliscco/interactive#19

Open

Adding cancel execution command #1057

Adding cancel execution command #1057

Uh oh!

Conversation

colombod commented Feb 10, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants