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.

Sign up for GitHub

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

Jump to bottom

UNIXSocket#recv_io, UNIXSocket#send_io implementation using JNR #3492

Merged
merged 2 commits into from
Dec 26, 2015
Merged

UNIXSocket#recv_io, UNIXSocket#send_io implementation using JNR #3492

merged 2 commits into from
Dec 26, 2015
+120 −12

Conversation

mrbrdo
Copy link
Contributor

@mrbrdo mrbrdo commented Nov 23, 2015

recv_io_spec.rb and send_io_spec.rb seem to now be passing. Also did a check with a script and seems to be working properly. Thorough review would be welcome, as this is my first JRuby PR.

Based on MRI code https://github.com/ruby/ruby/blob/trunk/ext/socket/unixsocket.c#L206
And on the test from jnr-posix: https://github.com/jnr/jnr-posix/blob/master/src/test/java/jnr/posix/IOTest.java#L163

I did skip this part: https://github.com/ruby/ruby/blob/af68619a6c0de0d4f08d092d65143abcf2cedce3/ext/socket/unixsocket.c#L355 (L355-L404)
I'm not sure if this is even relevant to JRuby nor do I completely understand what it is doing.

Cheers

@mrbrdo mrbrdo force-pushed the unixsocket_io branch 2 times, most recently from bb4e8f0 to eb646c3 Compare November 23, 2015 23:01
@kares kares added this to the JRuby 9.0.5.0 milestone Nov 24, 2015
@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 1, 2015

@headius do you need something else before this and #3494 can be merged?
Also could you add the 9.0.5.0 milestone to #3494 as well?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 7, 2015

@headius @kares ping

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 16, 2015

@headius @kares reping

@kares
Copy link
Member

kares commented Dec 16, 2015

sorry I can not decide whether its correct without any tests, try rebasing off a green build and un-exclude some of the MRI tests or add new ones

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 24, 2015

@kares @headius the MRI tests are checking IO#close_on_exec? which is not implemented on JRuby, so I cannot get those tests passing in their current form. The test_fd_passing test passes without this check. Even if we would hardcode that to false it would make the tests fail since they expect true.
PS: The method close_on_exec? is somewhat incorrectly throwing NotImplementedError: close_on_exec= (wrong method name is being shown)

Please advise?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 24, 2015

Also the failure in the tests above can't be my fault...

@headius
Copy link
Member

headius commented Dec 25, 2015

Sorry for the delay on this, @mrbrdo...travel and holidays tied me up I guess. We'll get this merged right away.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Dec 25, 2015

Thanks @headius , don't forget about #3494 :)

headius added a commit that referenced this pull request Dec 26, 2015
@headius
Merge pull request #3492 from mrbrdo/unixsocket_io
fe44f00 
UNIXSocket#recv_io, UNIXSocket#send_io implementation using JNR
@headius headius merged commit fe44f00 into jruby:master Dec 26, 2015
@rwilliams rwilliams mentioned this pull request Jan 9, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
core JRuby 9000
Projects
None yet
Milestone
JRuby 9.0.5.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mrbrdo @kares @headius