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.

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 reads after calling BasicSocket#close_write. #4503

Merged
merged 1 commit into from Feb 24, 2017

Conversation

@snowp
Copy link
Contributor

@snowp snowp commented Feb 24, 2017

shutdownInternal was always shutting down the read side, even when we were trying to close the write side of the socket.

Fixes #4500.

@snowp snowp changed the title Don't prevent reads after calling BasicSocket#close_write. Allow reads after calling BasicSocket#close_write. Feb 24, 2017
@snowp
Copy link
Contributor Author

@snowp snowp commented Feb 24, 2017

looking at it again im thinking the fptr.setMode(fptr.getMode() & ~closeHalf); in closeHalf might be redundant (since shutdownInternal nulls out the flag already). ill try it out in the morning

@kares kares added this to the JRuby 9.1.8.0 milestone Feb 24, 2017
@headius
Copy link
Member

@headius headius commented Feb 24, 2017

Ahh I had not seen you made a PR. Will review.

@headius
Copy link
Member

@headius headius commented Feb 24, 2017

You're right, that line is redundant if the contract of shutdownInternal is to also update read/write flags. Seems like that's appropriate, since I can't imagine why we'd want it NOT to do so.

I'll merge and remove the redundant line.

@headius headius merged commit 2558553 into jruby:master Feb 24, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
headius added a commit that referenced this pull request Feb 24, 2017
@nerdrew
Copy link

@nerdrew nerdrew commented Feb 24, 2017

@snowp THANKS!

@snowp snowp deleted the snowp:snowp/close_write branch Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.