-
Notifications
You must be signed in to change notification settings - Fork 602
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
Allow sending CHANNEL_EOF message when ChannelOutputStream is closed #554
Conversation
Codecov Report
@@ Coverage Diff @@
## master #554 +/- ##
============================================
- Coverage 56.37% 56.34% -0.04%
Complexity 1239 1239
============================================
Files 195 195
Lines 7904 7910 +6
Branches 722 723 +1
============================================
+ Hits 4456 4457 +1
- Misses 3097 3101 +4
- Partials 351 352 +1
Continue to review full report at Codecov.
|
@hierynomus from my perspective this PR is now ready - feel free to comment or to criticize it. As I wrote in my first post I don't see a good way to increase the code coverage because of the problems providing a platform independent unit test for this scenario. |
If I understand it correctly also bug #496 is also about the inability to send the CHANNEL_EOF when the OutputStream is closed. Therefore by applying this PR this would make it possible "fix" this issue, too. |
I am not sure what you mean with "race condition". The log of #143 does not contain many detailed error description. To me it is just a bunch of messages with works, works not, works again, works not... The only detailed description is your post - but if I understand it correctly this error happened when the "line" was commented out - which means that no EOF was send, correct? So the main question remains: Do you see the chance for a general solution? Because otherwise this workaround would at least to allow users to send EOF manually and therefore enable a lot of functionality which is currently unusable. |
I've run a small test connecting 1000 times and doing a command. I've decided to re-implement the fix in #143 unconditionally, and if there is a new bug, we'll have a look at it. So this PR can be closed I think. |
As discussed in #553 it is sometimes necessary to send an CHANNEL_EOF message while in other situation it is redundant.
Therefore the best solution in my opinion is to let the programmer choose if to send the CHANNEL_EOF message or not. Until now sending such a message requires some ugly hacks using reflection which is really bad. Therefore I implemented a new property that allows to enable sending an CHANNEL_EOF message when
close()
is called.It is designed to be used in this way:
Note: I did not provide any additional unit tests as it is pretty complicated to write unit test that uses this feature and works on all common platforms (Linux, Window,s OSX, ...) as it requires an command to be executed that waits for stdin like
cat
ortar x
. However not all platforms provide these commands by default, therefore the test would fail on at least one platform.