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.

Sign up for GitHub

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

Feature/change window size request for ssh client #1062

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

Feature/change window size request for ssh client #1062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6452fda
Added support for #1061 Window Change Request in ShellStream
Dec 11, 2022
485adff
#1061 Added not connected test case window-change:WindowChangeRequest…
Dec 11, 2022
a5d7b01
#1061 Added Request Failed and Request Successed test cases for windo…
Dec 11, 2022
ec60725
Removed extra whitespace before closing parenthesis
Dec 14, 2022
f77df6a
Ensure SendWindowChangeRequest is called once.
Dec 14, 2022
61d5324
Changed WindowChangeRequest to friendlier name.
Dec 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/Renci.SshNet.Tests/Classes/ShellStreamTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,37 @@ public void WriteLine_Line_ShouldOnlyWriteLineTerminatorWhenLineIsNull()
_channelSessionMock.Verify(p => p.SendData(lineTerminator), Times.Once);
}

[TestMethod]
public void WindowChangeRequest_ShouldReturnFalseWhenNotConnected()
{
var shellStream = CreateShellStream();
_channelSessionMock.Setup(s => s.IsOpen).Returns(false);
Assert.IsFalse(shellStream.ChangeWindow(80, 25, 0, 0));
}

[TestMethod]
public void WindowChangeRequest_ShouldReturnFalseWhenResultFails()
{
var shellStream = CreateShellStream();
_channelSessionMock.Setup(s => s.IsOpen).Returns(true);
_channelSessionMock.Setup(s => s.SendWindowChangeRequest(
Copy link
Member

Choose a reason for hiding this comment

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

Not very useful since SendWindowChangeRequest(...) always returns true, but let's discuss.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I dont think SendWindowChangeRequest SHOULD always be true. For example a change to size of 0,0,0,0 should fail and probably will on some pty implementations.

It.IsAny<uint>(), It.IsAny<uint>(),
It.IsAny<uint>(), It.IsAny<uint>())).Returns(false);
Assert.IsFalse(shellStream.ChangeWindow(80, 25, 0, 0));
}

[TestMethod]
public void WindowChangeRequest_ShouldReturnTrueWhenResultSucceeds()
{
var shellStream = CreateShellStream();
_channelSessionMock.Setup(s => s.IsOpen).Returns(true);
_channelSessionMock.Setup(s => s.SendWindowChangeRequest(
It.IsAny<uint>(), It.IsAny<uint>(),
It.IsAny<uint>(), It.IsAny<uint>())).Returns(true);
Assert.IsTrue(shellStream.ChangeWindow(80, 25, 0, 0));
_channelSessionMock.Verify(v => v.SendWindowChangeRequest(80,25,0,0), Times.Once());
}

private ShellStream CreateShellStream()
{
_sessionMock.Setup(p => p.ConnectionInfo).Returns(_connectionInfoMock.Object);
Expand Down
15 changes: 15 additions & 0 deletions src/Renci.SshNet/ShellStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
using System.Threading;
using System.Text.RegularExpressions;
using Renci.SshNet.Abstractions;
using System.CodeDom;
using System.Runtime.InteropServices;

namespace Renci.SshNet
{
Expand Down Expand Up @@ -52,6 +54,19 @@ public bool DataAvailable
}
}
}
/// <summary>
/// Sends a Window Change Request via the Channel.
/// </summary>
/// <param name="columns">New screen width in # of columns</param>
/// <param name="rows">New screen height in # of rows</param>
/// <param name="width">New screen width in Pixels</param>
/// <param name="height">New screen height in Pixels</param>
/// <returns>true when change is successful, or false when channel is NOT open or the request </returns>
public bool ChangeWindow(uint columns, uint rows, uint width, uint height )
{
if (_channel==null || !_channel.IsOpen) return false;
Copy link
Member

Choose a reason for hiding this comment

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

Please have it throw an ObjectDisposedException when _channel is null, and an InvalidOperationException if the channel is closed. Please document this as such and add corresponding unit tests.

I would've prefered to have the method on ChannelSession also throw if the channel were closed, but that would be considered a breaking change. That method currently either throws or returns true, never false. It also returns true if the channel is closed 😌

Copy link
Author

@glenkleidon glenkleidon Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This method is essentially a wrapper for SendWindowChangeRequest which returns a boolean. It makes sense that this method also returns boolean. Nothing will break if the request fails, so therefore it is not exceptional that the method fails.

I always ask myself - as library developers, can we KNOW that it is Exceptional that a method is called when in the wrong state or is it simply unexpected. Often the answer is that you can't know the answer because we don't know the context of how the method is being used. The developer using the library will know if the situation is exceptional and can throw if it is. All we need to do is say whether or not it succeeded.

If you throw an exception in a library - it has to be fundamentally wrong to do so. In this case, I don't think it is - it is merely unexpected and of no consequence. There wont be any data coming back to be "in the wrong shape" so it wont matter that the method failed. There might be an argument for Throwing the exception if the channel had never been opened, but as this happens in the constructor that is not possible.

The scenario that comes to mind is, in the CanClose Event of a Desktop App, OR the Close browser event, the shape of the window may change during clean up. If that happens, the event to change window shape MAY still be attached causing the event to fire when the channel has already closed. It is much simpler to handle for the developer if the method does not throw an exception and the unexpected event can be ignored. With an exception, in order to properly clean up, the developer must remember to unsubscribe to events or risk an exception being raised when it is of no consequence.

Returning false allows to the developer to decide if failed call to change window size is exceptional. So I think returning a boolean is the right call here.

Copy link
Author

@glenkleidon glenkleidon Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, ChangeWindow might be a better name. Happy to do this. Should I proceed?

Copy link
Author

Choose a reason for hiding this comment

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

Ok changed in commit [61d5324]

Copy link
Author

Choose a reason for hiding this comment

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

With regards to an example application - I have a fully worked example in branch glenkleidon:feature/add_vs_2022_solution

Should consider doing a pull request on that one but I need some help with a failed test case.

Copy link
Author

Choose a reason for hiding this comment

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

Actually the blocker is the failed test case on #1024

return _channel.SendWindowChangeRequest(columns, rows, width, height);
}

/// <summary>
/// Gets the number of bytes that will be written to the internal buffer.
Expand Down