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

Merged
merged 2 commits into from Dec 26, 2015

Conversation

Projects
None yet
3 participants
@mrbrdo
Contributor

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

@kares kares added this to the JRuby 9.0.5.0 milestone Nov 24, 2015

@mrbrdo

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 1, 2015

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 7, 2015

Contributor
Contributor

mrbrdo commented Dec 7, 2015

@mrbrdo

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 16, 2015

Contributor

@headius @kares reping

Contributor

mrbrdo commented Dec 16, 2015

@headius @kares reping

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Dec 16, 2015

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 24, 2015

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 24, 2015

Contributor

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

Contributor

mrbrdo commented Dec 24, 2015

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

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 25, 2015

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@mrbrdo

mrbrdo Dec 25, 2015

Contributor

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

Contributor

mrbrdo commented Dec 25, 2015

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

headius added a commit that referenced this pull request Dec 26, 2015

Merge pull request #3492 from mrbrdo/unixsocket_io
UNIXSocket#recv_io, UNIXSocket#send_io implementation using JNR

@headius headius merged commit fe44f00 into jruby:master Dec 26, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment