Skip to content

Fix ShellStream.Read returning 0 prematurely #923

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

Closed
wants to merge 3 commits into from

Conversation

IgorMilavec
Copy link
Collaborator

Fixes ShellStream.Read to block until more data is received or the session is closed.
This will fix #63

@IgorMilavec
Copy link
Collaborator Author

IgorMilavec commented Mar 18, 2022

@drieseng , I also have a fix ready for #672, which is based on this PR. Should I amend this PR or open a new one? I think the cleanest option would be if you review and merge this PR and I rebase and open a new one for #672. (edit: and #453, #670, #917, #833, #78, #140, #383)

@drieseng
Copy link
Member

@IgorMilavec This will block indefinitely if the last "line" does not end with a line ending. I have an integration test locally that allows you to reproduce this, but I'd really like you to have a go at this too. Let me know if you need any help or guidance.

@IgorMilavec
Copy link
Collaborator Author

ShellStream.Read() should be agnostic about any content, I don't see how it could fail in this way? Do you have an example so I can test it?

@drieseng
Copy link
Member

A few other remarks:

  • Read() still returns immediately.
  • Do we want a ReadTimeout setting (like you find on NetworkStream)?

@drieseng
Copy link
Member

@IgorMilavec Yes, I have a test for this but - as I said - I'd really prefer you also take a stab at this hereby hoping to convince you of the added value of these integration tests.

@drieseng
Copy link
Member

drieseng commented Mar 20, 2022

It reproduces when you use a StreamReader to read content of a ShellStream that is closed while reading.

@IgorMilavec
Copy link
Collaborator Author

Yes, I am aware that there are issues with closing the stream. Currently this PR only fixes #63. As I said, I already have other fixes ready, but I need your decision whether to add these changes to this PR?

@drieseng
Copy link
Member

@IgorMilavec We shouldn't merge this without a minimal fix that avoids a "hang" when the channel is closed, and corresponding tests.

@IgorMilavec
Copy link
Collaborator Author

@drieseng I have committed the changes required to handle session/channel closing. There were quite a bit of changes, mostly due to the fact that all Read* and Expect* methods directly handle internal state. Can you please have a look?

@IgorMilavec
Copy link
Collaborator Author

ShellStream has a race condition where only _incoming is locked and _dataReceived is not. Really solving this would require a substantial rewrite of inner workings. I tried to at least make the current implementation usable in b4ffc60, but introduced a regression in Expect (timeout never expired). This should now be fixed.

@glorialearnscoding
Copy link

Hello @IgorMilavec @drieseng , any plan to publish new version with bugs mentioned above fixed?

@TomBartDrone
Copy link

I repeat the @glorialearnscoding question, do you plan to publish fixes for that issues?

@jscarle
Copy link
Contributor

jscarle commented Feb 13, 2024

@jscarle Note to self.

@IgorMilavec I'd like to understand what your original plan was with this PR as ShellStream has changed enough that the diff is quite messy and I'd like to take a look at addressing this.

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.

ShellStream.Read not blocking when no data available
6 participants