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

Fix PipeStream.Read returning 0 prematurely #924

Closed
wants to merge 4 commits into from

Conversation

IgorMilavec
Copy link
Collaborator

@IgorMilavec IgorMilavec commented Feb 23, 2022

Fixes PipeStream.Read to block until more data is received or the session is closed.
This will fix #12, fix #164, fix #440, fix #650

@drieseng
Copy link
Member

drieseng commented Mar 5, 2022

@IgorMilavec I definitely want this change in, but there are still a few things to consider:

  • I don't think we deal with the situation where a connection or channel is closed while performing a blocking read.
  • Do we want to allow a (read) timeout to be configured?
  • Do we want to remove the BlockLastReadBuffer property?
  • I'd like a few integration tests that cover this. Should be pretty easy to add some for SshCommand.
    Perhaps a good way to familiarize yourself with (and/or criticize) the work I did in the *IntegrationTests repository.

@IgorMilavec
Copy link
Collaborator Author

PipeStream is used to communicate between two threads and is not related to a connection or channel. I don't even know why we need it, I just wanted to fix the issue reported. :)
BlockLastReadBuffer is definitely something I don't like. The way this is implemented interferes with detecting the end of stream. For "read the length I need and only return less data is end of stream is reached" functionality I propose this extension:

public static int ReadBlock(this Stream stream, byte[] buffer, int offset, int count)
{
    #region Validate arguments
    if (stream is null)
    {
        throw new ArgumentNullException(nameof(stream));
    }
    #endregion

    int bufferLength = 0;
    while (bufferLength < count)
    {
        int readLength = stream.Read(buffer, offset + bufferLength, count - bufferLength);
        if (readLength == 0)
        {
            break;
        }
        bufferLength += readLength;
    }

    return bufferLength;
}

Works for every stream, I also have an async version...

But generally, PipeStream is so terribly inefficient and complex that I propose to rewrite it completely. At this time I propose to only fix the most pressing bugs, release the final .NET 3.5 version and then refactor/rewrite such code when we only have to support modern frameworks.

@IgorMilavec
Copy link
Collaborator Author

IgorMilavec commented Mar 16, 2022

Just found out that ReadBlock() and ReadBlockAsync() will be added in dotnet 7: https://docs.microsoft.com/en-us/dotnet/api/system.io.streamreader.readblock?view=net-6.0.
So we can easily "pollyfill" them for earlier versions and use dotnet implementation from net7.0 onwards.
@drieseng should I add them in this PR and mark BlockLastReadBuffer deprecated?

I checked the integration tests and Ssh_Command_IntermittendOutput_OutputStream which is now ignored should now pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants