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

bpo-28724: Add methods send_fds, recv_fds to the socket module #12889

Merged
merged 27 commits into from Sep 11, 2019

Conversation

@nanjekyejoannah
Copy link
Contributor

nanjekyejoannah commented Apr 20, 2019

I have added send_fds(), recv_fds() to the socket module.

https://bugs.python.org/issue28724

@nanjekyejoannah nanjekyejoannah changed the title Add method send_fds, recv_fds to the socket module bpo-28724: Add method send_fds, recv_fds to the socket module Apr 20, 2019
@nanjekyejoannah nanjekyejoannah changed the title bpo-28724: Add method send_fds, recv_fds to the socket module bpo-28724: Add methodS send_fds, recv_fds to the socket module Apr 20, 2019
@nanjekyejoannah nanjekyejoannah changed the title bpo-28724: Add methodS send_fds, recv_fds to the socket module bpo-28724: Add methods send_fds, recv_fds to the socket module Apr 22, 2019
Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/test/test_socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
Lib/socket.py Outdated Show resolved Hide resolved
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented May 4, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@nanjekyejoannah nanjekyejoannah requested review from 1st1, asvetlov, ericvsmith, gpshead, pablogsal, rhettinger and python/email-team as code owners May 6, 2019
@nanjekyejoannah nanjekyejoannah force-pushed the nanjekyejoannah:issue28724 branch 3 times, most recently from 40d7ea4 to f3e0822 May 6, 2019
@requireAttrs(socket.socket, 'send_fds')
def testSendAndRecvFds(self):
fds = []
for i in range(1):

This comment has been minimized.

Copy link
@pablogsal

pablogsal May 10, 2019

Member

Why we need a loop of 1 iteration?

This comment has been minimized.

Copy link
@nanjekyejoannah

nanjekyejoannah May 17, 2019

Author Contributor

I want to return a list of 1 file descriptors for created files.

This comment has been minimized.

Copy link
@pablogsal

pablogsal May 17, 2019

Member

But you don't need a loop for that, right? Just substitute i by 0. I am missing something?

Lib/test/test_socket.py Outdated Show resolved Hide resolved
@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented May 27, 2019

Needs, Mac OS fixes. Will get a Mac to test this later.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Aug 24, 2019

@pablogsal , Please help me confirm If the tests pass on MacOS now. I cant access a Mac now.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Aug 24, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Aug 24, 2019

Thanks for making the requested changes!

@tiran, @pablogsal, @vstinner, @ncoghlan: please review the changes made to this pull request.

import array

def send_fds(sock, buffers, fds, flags=0, address=None):
""" send_fds(sock, buffers, fds[, flags[, address]]) -> socket object

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

It seems like sendmsg() returns an integer, not a socket object.

import array

def recv_fds(sock, bufsize, maxfds, flags=0):
""" recv_fds(sock, bufsize, maxfds[, flags]) -> (socket object, socket object)

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

The doc of the return value looks wrong. The code uses a tuple of 4 times: return msg, list(fds), flags, addr.

def recv_fds(sock, bufsize, maxfds, flags=0):
""" recv_fds(sock, bufsize, maxfds[, flags]) -> (socket object, socket object)
receive up to maxfds file descriptors returning the message

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

Upper case for the first word of the doc?

Suggested change
receive up to maxfds file descriptors returning the message
Receive up to maxfds file descriptors returning the message
with sock1, sock2:
socket.send_fds(sock1, [MSG], fds)
# request more data and file descriptors than expected
msg, fds2, flags, addr = socket.recv_fds(sock2, len(MSG) * 2, len(fds) * 2)

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

nitpick (PEP 8): there is an useless space

Suggested change
msg, fds2, flags, addr = socket.recv_fds(sock2, len(MSG) * 2, len(fds) * 2)
msg, fds2, flags, addr = socket.recv_fds(sock2, len(MSG) * 2, len(fds) * 2)
Receive up to *maxfds* file descriptors. Return ``(msg, list(fds), flags, addr)``. Consult
:meth:`recvmsg` for the documentation of these parameters.

.. availability:: Unix supporting :meth:`~socket.sendmsg` and :const:`SCM_RIGHTS` mechanism.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

it requires recvmsg() instead of sendmsg(), no?

.. method:: socket.send_fds(sock, buffers, fds[, flags[, address]])

Send the list of file descriptors *fds* over an :const:`AF_UNIX` socket.
Consult :meth:`sendmsg` for the documentation of these parameters.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 26, 2019

Member

You should document at least what is the fds parameter. I suggest to document it as sequence of file descriptors. Is it possible to send 0 file descriptor?

What is the return value?

Does someone know if it's possible that the call send only a few file descriptors, but not all of them? What if it's called with 100 000 file descriptors for example?

This comment has been minimized.

Copy link
@nanjekyejoannah

nanjekyejoannah Aug 28, 2019

Author Contributor

What is the return value?

The return type for sendmsg() is int AFAIK. Am not sure about these other edge cases but I can look.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 28, 2019

Member

I tested to send 1000 file descriptors: sendmsg() fails with "OSError: [Errno 22] Invalid argument".

For partial send, it can happen, as in socket.send(). In that case, according to https://stackoverflow.com/questions/38431713/partial-read-write-issue-in-sendmsg-recvmsg the file descriptors are already sent and should be be send again, socket.sendmsg() should be called again, but only with buffers (adjusted to the offset equal to the send_fds() result).

In short, you don't have to do anything. I'm not comfortable to document how to handle partial send since I'm not sure that the behavior is portable.

sendmsg() doesn't warn about partial send. Maybe we should copy send() warning into sendmsg() and send_fds(): "Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data."

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Aug 26, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Aug 28, 2019

I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Aug 28, 2019

Thanks for making the requested changes!

@ncoghlan, @pablogsal, @vstinner, @tiran: please review the changes made to this pull request.

Copy link
Member

vstinner left a comment

The test is skipped. When I enabled manually the test, it failed on Linux :-(

.. method:: socket.send_fds(sock, buffers, fds[, flags[, address]])

Send the list of file descriptors *fds* over an :const:`AF_UNIX` socket.
Consult :meth:`sendmsg` for the documentation of these parameters.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 28, 2019

Member

I tested to send 1000 file descriptors: sendmsg() fails with "OSError: [Errno 22] Invalid argument".

For partial send, it can happen, as in socket.send(). In that case, according to https://stackoverflow.com/questions/38431713/partial-read-write-issue-in-sendmsg-recvmsg the file descriptors are already sent and should be be send again, socket.sendmsg() should be called again, but only with buffers (adjusted to the offset equal to the send_fds() result).

In short, you don't have to do anything. I'm not comfortable to document how to handle partial send since I'm not sure that the behavior is portable.

sendmsg() doesn't warn about partial send. Maybe we should copy send() warning into sendmsg() and send_fds(): "Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data."

@requireAttrs(socket, "send_fds")
@requireAttrs(socket, "recv_fds")
@requireAttrs(socket, "AF_UNIX")
class SendRecvFdsTests(unittest.TestCase):

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 28, 2019

Member

Oh, this test case must be added manually into test_main(). Currently, the test is not run...

I added it manually and... the test fails :-( The test writes into the read end of the pipe, and reads from the write end of the pipe. It should be the opposite. Maybe on FreeBSD, pipe ends are not directional, but it seems to be the case on Linux.

So the test should be fixed on Linux a well.

Please move this class before test_main().

_socket.CMSG_LEN(maxfds * fds.itemsize))
for cmsg_level, cmsg_type, cmsg_data in ancdata:
if (cmsg_level == _socket.SOL_SOCKET and cmsg_type == _socket.SCM_RIGHTS):
# Append data, ignoring any truncated integers at the end.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 28, 2019

Member

@nanjekyejoannah: Would you mind to document this behavior in recv_fds() documention?

@@ -12,6 +12,8 @@
socket() -- create a new socket object
socketpair() -- create a pair of new socket objects [*]
fromfd() -- create a socket object from an open file descriptor [*]
send_fds() -- Send file descriptor to the socket.
recv_fds() -- Recieve file descriptor from the socket.

This comment has been minimized.

Copy link
@vstinner

vstinner Aug 28, 2019

Member

I suggest to replace "file descriptor" with "file descriptors" for both functions here (add an S), as in their name, since they can send and receive more than one file descriptor ;-)

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Aug 28, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Sep 9, 2019

The test is skipped. When I enabled manually the test, it failed on Linux :-(

test_main() is awful... and it's no longer needed: unittest.main() can be used instead.

I created https://bugs.python.org/issue38063 "Modify test_socket.py to use unittest test discovery" and I wrote a PR to implement it.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Sep 11, 2019

@vstinner

Test passes with your fix.

Ran 580 tests in 24.587s

OK (skipped=118)

== Tests result: SUCCESS ==

1 test OK.

Total duration: 24 sec 956 ms
Tests result: SUCCES

Also the test ir running:
testSendAndRecvFds (test.test_socket.SendRecvFdsTests) ... ok

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Sep 11, 2019

I tested manually commit 1ac7fbf on Linux and FreeBSD. I merged the PR with the master branch, make && ./python -m test test_socket -m SendRecvFdsTests -v . The test pass on Linux and FreeBSD. Good!

According to Azure Pipelines: test_socket also pass on macOS (and the test is no longer skipped on macOS).

test_socket pass on Windows as well, even if the test is skipped on Windows.

@vstinner vstinner merged commit 8d120f7 into python:master Sep 11, 2019
4 checks passed
4 checks passed
Azure Pipelines PR #20190911.196 succeeded
Details
bedevere/issue-number Issue number 28724 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nanjekyejoannah nanjekyejoannah deleted the nanjekyejoannah:issue28724 branch Sep 11, 2019
@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Sep 11, 2019

Thanks Shinya Okano for the original patch.

Well done Joannah @nanjekyejoannah! Thanks for your tenacity :-) This PR has 100 comments and 27 commits which shows the complexity of the feature.

Honestly, I'm not 100% happy with current documentation, but I chose to merge the PR anyway. Please leave https://bugs.python.org/issue28724 open until the documentation is completed to mention corner cases like partial send (similar to sock.send vs sock.sendall).

vrpolakatcisco added a commit to vrpolakatcisco/cpython that referenced this pull request Sep 12, 2019
…thonGH-12889)

The socket module now has the socket.send_fds() and socket.recv.fds() functions.
Contributed by Joannah Nanjekye, Shinya Okano (original patch)
and Victor Stinner.

Co-Authored-By: Victor Stinner <vstinner@redhat.com>
DinoV added a commit to DinoV/cpython that referenced this pull request Sep 12, 2019
…thonGH-12889)

The socket module now has the socket.send_fds() and socket.recv.fds() functions.
Contributed by Joannah Nanjekye, Shinya Okano (original patch)
and Victor Stinner.

Co-Authored-By: Victor Stinner <vstinner@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.