Prevent AnonymousKernelCommands from ending up inside serialized KernelEvents #2888
Conversation
…elEvents. AnonymousKernelCommands are not supposed to be serialized across process boundaries and should therefore not be included as part of KernelEvents.
shyamnamboodiripad
force-pushed
the
anonymous
branch
from
5d4aef5 to
bc8e36c
Compare
| @@ -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() | |||
There was a problem hiding this comment.
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
456This should apply to command types including various language service commands.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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!
| @@ -160,7 +160,6 @@ private bool CanHandleLocally(KernelCommand command) | |||
| switch (command) | |||
| { | |||
| case AnonymousKernelCommand: | |||
| return base.HandleAsync(command, context); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@colombod Agreed. This is part of what Jon and I discussed, and I have included the larger problem in this follow up issue - #2884 which I plan to look into soon. This PR addresses one known case where AnonymousKernelCommand ends up leaving the current host today (see the fix in SubmissionParser.cs line 127 in the current PR). The plan that @jonsequitur and I discussed was to merge this fix now and tackle the larger issue in subsequent PR. I was essentially waiting for him to take a second look and approve the PR today. Note that I was planning to merge this change today since this is one of the issues that came up when testing the VS side integration. So would appreciate if you could remove the block on the PR :) |
|
As a tactical solution I would rather add to the case a check to see if the handler is defined or not, if the anonymous command has a handler execute it otherwise skip it (this forces the command to stay in the current host and not leaving the proxy) . |
|
Let's find some time to discuss how we want to do this while removing rather than adding code. There's too much complexity here. |
AnonymousKernelCommands are not supposed to be serialized across process boundaries and should therefore not be included as part of KernelEvents.