Skip to content

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
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