Skip to content

Improve ShellStream Expect #1315

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

Merged

Conversation

WojciechNagorski
Copy link
Collaborator

There are three implementation of Expect method:

public string Expect(Regex regex, TimeSpan timeout);
public void Expect(TimeSpan timeout, params ExpectAction[] expectActions);
public IAsyncResult BeginExpect(TimeSpan timeout, AsyncCallback callback, object state, params ExpectAction[] expectActions);

Two of them return string ends with the expected expression.
Only the first one works differently. In this PR I fixed this problem.

Moreover, the _incoming array contains bytes and regex works on UTF8 encoding, so we should count how many bytes we should remove from the incoming array.

Continuation of #1313
Added test failures for #1207 (does not fix performance issue)

@WojciechNagorski
Copy link
Collaborator Author

@Rob-Hague Can I ask for a review?

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.

Thanks

I have nearly fixed the remaining tests

@WojciechNagorski
Copy link
Collaborator Author

@Rob-Hague Thanks! I haven't touched the other tests and I won't.

@WojciechNagorski WojciechNagorski merged commit 3bfac50 into sshnet:develop Feb 11, 2024
@WojciechNagorski WojciechNagorski deleted the Fix-ShellStream-Expect branch February 11, 2024 22:06
@@ -471,6 +475,7 @@ public IAsyncResult BeginExpect(TimeSpan timeout, AsyncCallback callback, object
if (match.Success)
{
var result = text.Substring(0, match.Index + match.Length);
var charCount = _encoding.GetByteCount(result);

for (var i = 0; i < match.Index + match.Length && _incoming.Count > 0; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line use charCount?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@jscarle jscarle Feb 11, 2024

Choose a reason for hiding this comment

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

I fixed it in my PR.

@Rob-Hague
Copy link
Collaborator

Btw, working on ShellStream makes me wish it derived from StreamReader rather than Stream... it would save a lot of repeated encoding work for the Expect methods (and just make more sense in general). But perhaps not worth the break.

@jscarle
Copy link
Contributor

jscarle commented Feb 11, 2024

I believe that ShellStream still has it's place, however perhaps a new ShellStreamReader could be added that would consume the a ShellStream and we could slowly migrate functionality between the two classes.

It could even be done in two steps. First by adding the new methods into ShellStreamReader, and then later deprecating them on ShellStream.

@WojciechNagorski
Copy link
Collaborator Author

This issue has been fixed in the 2024.0.0 version.

@WojciechNagorski WojciechNagorski added this to the 2024.0.0 milestone Feb 22, 2024
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.

3 participants