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

Improve shell stream #1514

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

Conversation

294797392
Copy link

Improve ShellStream

  • Added SendWindowChangeRequest to ShellStream

  • Added a new constructor to ShellStream

  • SshClient adds an overload function for CreateShellStream

When creating ShellStream, the channel will be immediately opened in the constructor of ShellStream. At this time, the calling module may not have subscribed to ShellStream's DataReceived event, which may result in the loss of a small piece of data immediately after connection.
You can use ShellStream's new constructor to solve this problem

* Added SendWindowChangeRequest to ShellStream

* Added a new constructor to ShellStream

When creating ShellStream, the channel will be immediately opened in the constructor of ShellStream. At this time, the calling module may not have subscribed to ShellStream's DataReceived event, which may result in the loss of a small piece of data immediately after connection.
You can use ShellStream's new constructor to solve this problem
* Added SendWindowChangeRequest to ShellStream

* Added a new constructor to ShellStream

* SshClient adds an overload function for CreateShellStream

When creating ShellStream, the channel will be immediately opened in the constructor of ShellStream. At this time, the calling module may not have subscribed to ShellStream's DataReceived event, which may result in the loss of a small piece of data immediately after connection.
You can use ShellStream's new constructor to solve this problem
@294797392
Copy link
Author

Why does AppVeyor fail?

@Rob-Hague
Copy link
Collaborator

Hi @294797392

Are you able solve your problem using the Read method instead of the DataReceived event? e.g.

byte[] buffer = new byte[1024];
int bytesRead;
while ((bytesRead = shellStream.Read(buffer) != 0)
{
    // Process "bytesRead" bytes from buffer
}

@294797392
Copy link
Author

294797392 commented Oct 9, 2024

Yes, using the Read function can solve my problem.
But I think it's best to optimize it, as other developers may also encounter this issue when using it.
Do you think it is necessary to add the SendWindowChangeRequest function?
@Rob-Hague

@Rob-Hague
Copy link
Collaborator

Yes it seems reasonable to add SendWindowChangeRequest - there have been previous requests for it. Personally I would suggest to:

  1. make it return void: the server does not respond to a window-change request, and our implementation in ChannelSession just returns true so it is not much use to return bool
  2. add ThrowHelper.ThrowObjectDisposedIf(_disposed, this); at the start of the method
  3. add at least a basic test. For example, in test\Renci.SshNet.Tests\Classes\ShellStreamTest.cs there could be a test which verifies the call to IChannelSession mock, and verifies the ObjectDisposedException when the ShellStream is closed

Let me know what you think

@294797392
Copy link
Author

294797392 commented Oct 11, 2024

I agree with your idea.
I tried to modify the code and added the SendWindowChangeRequest method, but found that if it returns void, IDE0058 compilation error will occur.
If void is returned, there may be many modifications and it will also affect the consistency of the code (there are many methods in ChannelSession that directly return true). Perhaps having a return value would be better than having no return value, as a return value can prompt the caller about the execution result of the method you called, and in the future we can also add more error handling to the method (if necessary)!
And I believe that when developing the ChannelSession module initially, there must have been a reason for returning a boolean.
@Rob-Hague

@Rob-Hague
Copy link
Collaborator

Ok, bool seems fine to me - I don't feel strongly about it

@294797392
Copy link
Author

Ok, bool seems fine to me - I don't feel strongly about it

Do you agree with me changing it to a boolean return value? If you agree, I will submit another PR

@Rob-Hague
Copy link
Collaborator

Yes

…dWindowChangeRequest

2. Added SendWindowChangeRequest_ThrowsObjectDisposedException to ShellStreamTest
Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

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

Could you please change ChannelSession.SendWindowChangeRequest to call TrySendMessage instead of SendMessage?

@@ -123,6 +123,18 @@ public void Write_AfterDispose_ThrowsObjectDisposedException()
Assert.ThrowsException<ObjectDisposedException>(() => shellStream.Write(bytes, 0, bytes.Length));
}

[TestMethod]
public void SendWindowChangeRequest_ThrowsObjectDisposedException()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you also add a simple test which verifies the call to ChannelSession.SendWindowChangeRequest?

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

Successfully merging this pull request may close these issues.

2 participants