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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

HarleyRossetto
Copy link

Resolves #76436

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 15, 2024
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Dec 15, 2024
@HarleyRossetto

This comment was marked as resolved.

@HarleyRossetto
Copy link
Author

@JoeRobich re: this comment: #72871 (comment)

Where would you guys like to redirect Console.Out to?

@CyrusNajmabadi
Copy link
Member

Can we do this as well:

One thought is that we would use the stream returned from Console.OpenStandardOutput() for messaging and reassign Console.Out using Console.SetOut() to redirect Console.WriteLine() calls to a different stream.

@HarleyRossetto
Copy link
Author

HarleyRossetto commented Dec 15, 2024

Can we do this as well:

One thought is that we would use the stream returned from Console.OpenStandardOutput() for messaging and reassign Console.Out using Console.SetOut() to redirect Console.WriteLine() calls to a different stream.

Yeah, whats the preferred destination?
Do we want a temporary file? stderr?

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?

@JoeRobich
Copy link
Member

Yeah, whats the preferred destination?
Do we want a temporary file? stderr?

I think ideally it would be Stream.Null. I don't think that we would want to always write to a temporary file, although it could be useful when troubleshooting. StandardError seems like a good in-between place to land, although I've certainly gotten feedback about writing non-error messages to stderr before. @jasonmalinowski what are your thoughts?

@HarleyRossetto
Copy link
Author

Yeah, whats the preferred destination?
Do we want a temporary file? stderr?

I think ideally it would be Stream.Null. I don't think that we would want to always write to a temporary file, although it could be useful when troubleshooting. StandardError seems like a good in-between place to land, although I've certainly gotten feedback about writing non-error messages to stderr before. @jasonmalinowski what are your thoughts?

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.

@jasonmalinowski
Copy link
Member

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?

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.

@JoeRobich
Copy link
Member

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. :-)

Well we won't reference Console.Out directly but will get the stream via Console.OpenStandardOutput() which should always return the correct output stream.

@HarleyRossetto Sound like we can move forward with redirecting to stderr.

@HarleyRossetto
Copy link
Author

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.
Copy link
Member

@JoeRobich JoeRobich left a 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);
Copy link
Member

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.

Comment on lines +132 to +133
// Redirect Console.Out to try prevent the standard output stream from being corrupted.
Console.SetOut(new StreamWriter(Console.OpenStandardError()));
Copy link
Member

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.

Copy link
Author

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()));
    }
    
    ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roslyn-lsp --stdio option
4 participants