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.

Sign up for GitHub

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

Jump to bottom

Prevent AnonymousKernelCommands from ending up inside serialized KernelEvents #2888

Merged
merged 2 commits into from
Apr 7, 2023
Merged

Prevent AnonymousKernelCommands from ending up inside serialized KernelEvents #2888

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc8e36c
Prevent AnonymousKernelCommands from ending up inside serialized Kern…
shyamnamboodiripad Apr 6, 2023
d97d5c9
Revert changes in KernelEvent.cs.
shyamnamboodiripad Apr 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/Microsoft.DotNet.Interactive.Tests/Parsing/SubmissionParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using FluentAssertions.Execution;
using Microsoft.DotNet.Interactive.Commands;
using Microsoft.DotNet.Interactive.CSharp;
using Microsoft.DotNet.Interactive.Events;
using Microsoft.DotNet.Interactive.FSharp;
using Microsoft.DotNet.Interactive.Jupyter;
using Microsoft.DotNet.Interactive.Parsing;
Expand Down Expand Up @@ -337,7 +338,7 @@ public void root_node_span_always_expands_with_child_nodes()
.Should()
.AllSatisfy(child => rootSpan.Contains(child.Span).Should().BeTrue());
}

private static SubmissionParser CreateSubmissionParser(string defaultLanguage = "csharp")
{
using var compositeKernel = new CompositeKernel
Expand Down Expand Up @@ -365,6 +366,15 @@ private static SubmissionParser CreateSubmissionParser(string defaultLanguage =
return compositeKernel.SubmissionParser;
}

[Fact]
public async Task DiagnosticsProduced_events_always_point_back_to_the_original_command()
Copy link
Contributor

@jonsequitur jonsequitur Apr 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is intended to be generally true, so not specific to DiagnosticsProduced. The internal command (AnonymousKernelCommand in this case, but it could as easily be one of the public command types) is an implementation detail that the external caller shouldn't have to be aware of.

It might be useful to add another similar test for the split between subkernels because the same should be true for, for example:

#!csharp
123
#!fsharp
456

This should apply to command types including various language service commands.

Copy link
Contributor Author

@shyamnamboodiripad shyamnamboodiripad Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So looks like this invariant is not satisfied for all events in the above case. For example, CodeSubmissionReceived, CompleteCodeSubmissionReceived, ReturnValueProduced events for the repro code you shared above point back to the per-language SubmitCode commands created internally for C# and F# respectively. Only the final CommandSucceded event points back to the original SubmitCode command.

If I change the submission to the following then interestingly, the DiagnosticsProduced event for the erroneous line points to the F#-specific SubmitCode command with code some error. However, the line spans in the DiagnosticsProduced.Diagnostics reflect the spans in the original submission.

#!csharp
123
#!fsharp
some error

This may be reasonable considering that the inner commands reflect the expected command splitting behavior - however, it also seems like internal implementation details of the callee are bleeding through to the caller.

If this is unexpected, I can log a separate bug to a) fix the behavior and b) add tests to ensure that inner commands are never exposed to the caller. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonsequitur Btw the current PR should be ready to merge. The above issue needs more investigation / follow-up and I'd prefer not to hold up the current PR for that. Please approve if you agree :) Thanks!

{
using var kernel = new CSharpKernel();
var command = new SubmitCode("#!unrecognized");
var result = await kernel.SendAsync(command);
result.Events.Should().ContainSingle<DiagnosticsProduced>().Which.Command.Should().BeSameAs(command);
}

[Fact]
public async Task ParsedDirectives_With_Args_Consume_Newlines()
{
Expand Down Expand Up @@ -447,7 +457,7 @@ public void RequestDiagnostics_can_be_split_into_separate_commands()
// language-specific code";

MarkupTestFile.GetLineAndColumn(markupCode, out var code, out var _, out var _);

var command = new RequestDiagnostics(code);
var commands = new CSharpKernel().UseDefaultMagicCommands().SubmissionParser.SplitSubmission(command);

Expand Down
1 change: 0 additions & 1 deletion src/Microsoft.DotNet.Interactive/Connection/ProxyKernel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ internal override Task HandleAsync(
switch (command)
{
case AnonymousKernelCommand:
return base.HandleAsync(command, context);
Copy link
Member

Choose a reason for hiding this comment

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

this could lead to a deadlock if the anonymous command targets the proxy. For sure this will throw exception as it will try to forward the command on the sender and will fail at serialization. This looks like a test gap to pinpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this PR is fixing one specific case where we were serializing AnonymousKernelCommand (inside SubmissionParser.cs). But the larger test gap has not been addressed yet - I have included this in #2884 and plan to address that in a follow up PR.

case DirectiveCommand:
return base.HandleAsync(command, context);
}
Expand Down
12 changes: 11 additions & 1 deletion src/Microsoft.DotNet.Interactive/Events/KernelEvent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@ public abstract class KernelEvent
{
protected KernelEvent(KernelCommand command)
{
Command = command ?? throw new ArgumentNullException(nameof(command));
if (command is null)
{
throw new ArgumentNullException(nameof(command));
}

if (command is AnonymousKernelCommand)
shyamnamboodiripad marked this conversation as resolved.
Show resolved Hide resolved
{
throw new ArgumentException($"{nameof(command)} should not be an {nameof(AnonymousKernelCommand)}");
}

Command = command;
RoutingSlip = new EventRoutingSlip();
}

Expand Down
30 changes: 14 additions & 16 deletions src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ public IReadOnlyList<KernelCommand> SplitSubmission(RequestDiagnostics requestDi
requestDiagnostics,
requestDiagnostics.Code,
(languageNode, parent, _) => new RequestDiagnostics(languageNode, parent));
return commands.Where(c => c is RequestDiagnostics ).ToList();

return commands.Where(c => c is RequestDiagnostics).ToList();
}

private delegate KernelCommand CreateChildCommand(
Expand All @@ -88,7 +88,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
var nodes = tree.GetRoot().ChildNodes.ToArray();

var targetKernelName = originalCommand.TargetKernelName ?? DefaultKernelName();

var lastCommandScope = originalCommand.SchedulingScope;
KernelNameDirectiveNode lastKernelNameNode = null;

Expand All @@ -97,11 +97,11 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
switch (node)
{
case DirectiveNode directiveNode:
if (KernelInvocationContext.Current is {} context)
if (KernelInvocationContext.Current is { } context)
{
context.CurrentlyParsingDirectiveNode = directiveNode;
}

var parseResult = directiveNode.GetDirectiveParseResult();

if (parseResult.Errors.Any())
Expand All @@ -124,7 +124,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
CodeAnalysis.DiagnosticSeverity.Error,
"NI0001", // QUESTION: (SplitSubmission) what code should this be?
"Unrecognized magic command");
var diagnosticsProduced = new DiagnosticsProduced(new[] { diagnostic }, c);
var diagnosticsProduced = new DiagnosticsProduced(new[] { diagnostic }, originalCommand);
shyamnamboodiripad marked this conversation as resolved.
Show resolved Hide resolved
context.Publish(diagnosticsProduced);
return Task.CompletedTask;
});
Expand All @@ -141,7 +141,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
{
commands.Clear();

if (sendExtraDiagnostics is {})
if (sendExtraDiagnostics is { })
{
commands.Add(sendExtraDiagnostics);
}
Expand Down Expand Up @@ -214,7 +214,6 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
break;

case LanguageNode languageNode:
{
if (commands.Count > 0 &&
commands[^1] is SubmitCode previous)
{
Expand All @@ -230,7 +229,6 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
commands.Add(command);
}
}
}
break;

default:
Expand Down Expand Up @@ -258,7 +256,7 @@ private IReadOnlyList<KernelCommand> SplitSubmission(
if (NoSplitWasNeeded())
{
originalCommand.TargetKernelName ??= targetKernelName;
return new []{originalCommand};
return new[] { originalCommand };
}

foreach (var command in commands)
Expand Down Expand Up @@ -291,8 +289,8 @@ bool NoSplitWasNeeded()
}
}

if (commands.All(c => c.GetType() == originalCommand.GetType() &&
(c.TargetKernelName == originalCommand.TargetKernelName
if (commands.All(c => c.GetType() == originalCommand.GetType() &&
(c.TargetKernelName == originalCommand.TargetKernelName
|| c.TargetKernelName == commands[0].TargetKernelName)))
{
return true;
Expand Down Expand Up @@ -324,7 +322,7 @@ private string DefaultKernelName()
CompositeKernel c => c.DefaultKernelName,
_ => _kernel.Name
};

return kernelName;
}

Expand All @@ -335,7 +333,7 @@ private string DefaultKernelName()
return null;
}

var dict = new Dictionary<string, (SchedulingScope , Func<Parser>)>();
var dict = new Dictionary<string, (SchedulingScope, Func<Parser>)>();

foreach (var childKernel in compositeKernel.ChildKernels)
{
Expand All @@ -347,7 +345,7 @@ private string DefaultKernelName()
}
}

(SchedulingScope, Func<Parser>) GetParser() => (childKernel.SchedulingScope,() => childKernel.SubmissionParser.GetDirectiveParser());
(SchedulingScope, Func<Parser>) GetParser() => (childKernel.SchedulingScope, () => childKernel.SubmissionParser.GetDirectiveParser());
}

return dict;
Expand All @@ -374,7 +372,7 @@ internal Parser GetDirectiveParser()
typeof(KernelInvocationContext),
_ => KernelInvocationContext.Current);
});

_directiveParser = commandLineBuilder.Build();
}

Expand Down