-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add option for choosing stdio as the LSP communication channel #76437
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
@JoeRobich re: this comment: #72871 (comment) Where would you guys like to redirect Console.Out to? |
Can we do this as well:
|
Yeah, whats the preferred destination? Do we also need to do anything with the logger factory and redirect that too or will simply redirecting Console.Out flow through to that too? |
I think ideally it would be |
I also had the thought of outputting to another long lived log file under the extension log directory path today, so just throwing that out there too. Will await Jason's thought on all of this. |
We'd have to very carefully check on the ordering that everything gets created to make sure it'd grab Console.Out at the right time. We should most definitely add a test for that. :-) Writing out stderr all the logging/errors/whatever-else-people-tried-writing-to-stdout seems reasonable to me, especially because you'd want somewhere to write if things go really bad and you can't get LSP started. Honestly, stderr might be where the runtime might send stuff regardless. |
Well we won't reference @HarleyRossetto Sound like we can move forward with redirecting to stderr. |
Ok thanks for everyone's input, at work currently, will get to it this evening. You should have something that review tomorrow. |
Redirecting Console.Out to use the standard error stream to try reduce the chances of stdout being corrupted by non-rpc content.
bd9ab58
to
07929ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating @HarleyRossetto! Will likely be a couple weeks before we can look at moving this forward. Most of the team will be out until the new year.
@@ -261,6 +280,7 @@ static CliRootCommand CreateCommandLineParser() | |||
var razorDesignTimePath = parseResult.GetValue(razorDesignTimePathOption); | |||
var extensionLogDirectory = parseResult.GetValue(extensionLogDirectoryOption)!; | |||
var serverPipeName = parseResult.GetValue(serverPipeNameOption); | |||
var useStdIo = parseResult.GetValue(useStdIoOption); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add validation that --stdio
and --pipe
are not both passed together. Logging an error and returning a non-zero exit code would be appropriate.
// Redirect Console.Out to try prevent the standard output stream from being corrupted. | ||
Console.SetOut(new StreamWriter(Console.OpenStandardError())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this redirection to the top of RunAsync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this point, should it be just after the logger is created?
static async Task RunAsync(ServerConfiguration serverConfiguration, CancellationToken cancellationToken)
{
// Create a console logger as a fallback to use before the LSP server starts.
using var loggerFactory = LoggerFactory.Create(builder => ...);
var logger = loggerFactory.CreateLogger<Program>();
// Handle LSP communication channel method and Console.Out redirection
if (serverConfiguration.UseStdIo)
{
if (serverConfiguration.ServerPipeName is not null)
{
logger.LogError("Server cannot be started with both --stdio and --pipe options.");
Environment.Exit(1);
}
// Redirect Console.Out to try prevent the standard output stream from being corrupted.
Console.SetOut(new StreamWriter(Console.OpenStandardError()));
}
...
Resolves #76436