bpo-32373: Add socket.getblocking() method. #4926

1st1 · 2017-12-19T14:24:46Z

https://bugs.python.org/issue32373

vstinner

Please add self.assertFalse(self.serv.getblocking()) in testInitNonBlocking(), or write a new test to ensure that socket created with SOCK_NONBLOCK is non-blocking: getblocking() is false.

vstinner · 2017-12-19T14:39:42Z

Doc/library/socket.rst

+
+   Return ``True`` if socket is in blocking mode, ``False`` if in
+   non-blocking.  This reflects the last call to :meth:`setblocking`
+   or :meth:`settimeout`.


"This reflects the last call to :meth:setblocking or :meth:settimeout."

What abou the following case?

>>> import socket; socket.socket(type=socket.SOCK_STREAM | socket.SOCK_NONBLOCK).getblocking() False

I suggest to remove the second sentence.

1st1 · 2017-12-19T14:46:19Z

Please add self.assertFalse(self.serv.getblocking()) in testInitNonBlocking(), or write a new test to ensure that socket created with SOCK_NONBLOCK is non-blocking: getblocking() is false.

There're extensive tests for that, see the updated NonblockConstantTest. But I've added it to testInitNonBlocking just in case.

vstinner

LGTM. You may document the new change in Doc/whatsnew/3.7.rst.

asvetlov · 2017-12-19T16:57:38Z

Modules/socketmodule.c

+PyDoc_STRVAR(getblocking_doc,
+"getblocking()\n\
+\n\
+Returns True if socket is in blocking mode, or False if it\n\


Very minor thing: moving if it to next line increases docstrict readability imho.

vstinner · 2017-12-19T20:08:33Z

Modules/socketmodule.c

@@ -2583,7 +2605,7 @@ sock_settimeout(PySocketSockObject *s, PyObject *arg)
        return NULL;

    s->sock_timeout = timeout;
-    if (internal_setblocking(s, timeout < 0) == -1) {
+    if (internal_setblocking(s, timeout != 0) == -1) {


Hum, wait, what is this change? Timeouts > 0 now set the socket is blocking mode?

Would you mind to document this subtle behaviour change in settimeout() documentation?

Yes. I'm still working on it. Turns out sendfile currently tests test sendfile on non-blocking sockets, but at the same time the documentation says that it's not supported for non-blocking sockets at all. It's a mess.

1st1 · 2017-12-19T23:39:07Z

@vstinner Everything is back to normal, the patch is simple again. Please read the documentation update in adcc91f.

pitrou · 2017-12-19T23:48:43Z

Lib/test/test_socket.py

+                # If timeout > 0, the socket will be in a "blocking" mode
+                # from the standpoint of the Python API.  For Python socket
+                # object, "blocking" means that operations like 'sock.recv()'
+                # will block.  Internally, file descriptors for


I don't think there's any point in explaining this inside test_socket.

In this test it makes sense, because I use fcntl to check the blocking mode of the underlying FD. Usually comments don't hurt in tests.

pitrou · 2017-12-19T23:49:45Z

Modules/socketmodule.c

-    if (internal_setblocking(s, timeout < 0) == -1) {
+
+    int block = timeout < 0;
+    /* Blocking mode for a Python socket object means that operations


Hmm, I'm sure that comment can be made much shorter :-)

Can you write your version?

Sure:

/* When a socket socket has a non-infinite timeout, * its underlying fd is internally set to non-blocking. * This table summarizes [etc.] */

Hm, that's too short :) But I'll trim down my version.

Pushed an update.

pitrou · 2017-12-19T23:50:28Z

Doc/library/socket.rst

+   a :exc:`BlockingIOError` if they cannot complete immediately.
+
+   Internally, if a Python socket object has a positive timeout, its
+   underlying file descriptor (FD) is switched to a non-blocking mode.


I'm wary of documenting internals here. It could be quite confusing to the reader, and doesn't seem to provide any useful information.

Yeah, but my confusion with this thing came from me trying to check if the FD is blocking/nonblocking with fcntl.fcntl(sock, fcntl.F_GETFL, os.O_NONBLOCK) & os.O_NONBLOCK, and from there it's was quite a painful experience to discover how socket objects really work in Python (probably because I was debugging a failing sendfile test after some changes, but whatever).

I agree that all this information might overwhelm an average Python user who just wants to read the documentation. On the other hand, socket module is usually used by more or less advanced Python users.

That said, I'd be OK to keep this detailed documentation piece in socketmodule.c if everyone thinks it's too much to have it here.

I think it would be best located in socketmodule.c indeed. Not sure where.

Let's keep it in sock_settimeout(), as it's the central place that triggers blocking/non-blocking modes.

I remove my vote since Yury rewrote his PR after I approved it

vstinner

I don't think that getblocking() doc is the right place to explain in depth the behaviour of all timeouts and blocking/non-blocking modes.

getblocking() should be a simple as: return True if the timeout is equal to zero, False otherwise.

Maybe the long description of blocking/non-blocking should even be a separated PR?

bedevere-bot · 2017-12-19T23:54:39Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2017-12-19T23:58:37Z

Misc/NEWS.d/next/Library/2017-12-19-09-23-46.bpo-32373.8qAkoW.rst

@@ -0,0 +1 @@
+Add socket.getblocking() method.


It seems like you modified settimeout() behaviour. It must be documented here and in settimeout() doc.

I'm wrong, settimeout() is (no longer) modified by this PR.

My comment was for a previous version of the PR. The PR evolved too quickly, I was lost, sorry.

1st1 · 2017-12-20T00:12:30Z

I remove my vote since Yury rewrote his PR after I approved it

The PR is in the exact same shape as it was the moment you approved it + few comments. I've pushed a few commits experimenting with different approaches, and later removed those experimental commits.

getblocking() should be a simple as: return True if the timeout is equal to zero, False otherwise.

That's exactly what is in the PR.

vstinner · 2017-12-20T00:29:07Z

Misc/NEWS.d/next/Library/2017-12-19-09-23-46.bpo-32373.8qAkoW.rst

@@ -0,0 +1 @@
+Add socket.getblocking() method.


I'm wrong, settimeout() is (no longer) modified by this PR.

My comment was for a previous version of the PR. The PR evolved too quickly, I was lost, sorry.

vstinner · 2017-12-20T00:35:26Z

Doc/library/socket.rst

+.. method:: socket.getblocking()
+
+   Return ``True`` if socket is in blocking mode, ``False`` if in
+   non-blocking.


Victor: "getblocking() should be a simple as: return True if the timeout is equal to zero, False otherwise."

Yury: "That's exactly what is in the PR."

Hum, this doc doesn't mention the timeout but refers to a "blocking mode" which is not well defined for timeout > 0.

The socket module mentions 3 modes: blocking, non-blocking and "timeout mode":
https://docs.python.org/dev/library/socket.html#notes-on-socket-timeouts

Replace "if socket is in blocking mode" with "if the timeout is equal to zero", or replace "is in blocking mode" with "is in blocking or timeout mode"?

By the way, there is already a note: "At the operating system level, sockets in timeout mode are internally set in non-blocking mode."

I think the docstring here should say "This is equivalent to gettimeout() == 0", just like the docstring for setblocking explains that it's equivalent to a certain settimeout call.

vstinner · 2017-12-20T00:40:09Z

Lib/test/test_socket.py

        self.serv.setblocking(False)
        self.assertEqual(self.serv.gettimeout(), 0.0)
+        self.assertFalse(self.serv.getblocking())
+        if fcntl:
+            self.assertFalse(_is_fd_in_blocking_mode(self.serv))


You may write an helper function which makes sure that timeout, blocking and SOCK_NONBLOCK flag are consistent.

Something like:

def check_timeout(sock, timeout): assert sock.gettimeout() == timeout assert sock.getblocking() == (timeout == 0.0) assert _is_fd_in_blocking_mode(sock) == (timeout != 0.0)

(I'm not sure of these assertions :-))

njsmith · 2017-12-20T00:53:47Z

Modules/socketmodule.c

+       ==================== ===================== ==============
+        ``None``             ``True``              blocking
+        ``0.0``              ``False``             non-blocking
+        ``> 0``              ``True``              non-blocking


Since this is internal C docs, it might be helpful to add a column to the table for s->sock_timeout values – IIUC the rows would be < 0, 0.0, and > 0 respectively, but it took some grovelling through the source to figure that out.

1st1 · 2018-01-28T20:22:15Z

@vstinner Do you have any objections here? I'd like to merge this to 3.7 and I don't see any open questions regarding this PR. Reorganizing tests to make them prettier is fine, but we can surely do it later.

vstinner

LGTM.

If I recall correctly, I voted -1 on a previous because setblocking() was modified. It's not more the case. Moreover, the new doc is also better now.

vstinner · 2018-01-28T22:26:51Z

Modules/socketmodule.c

+"getblocking()\n\
+\n\
+Returns True if socket is in blocking mode, or False if it\n\
+is in non-blocking mode.");


You may add "This is equivalent to checking socket.gettimeout() == 0." here as well.

bedevere-bot · 2018-01-28T22:27:40Z

@1st1: Please replace # with GH- in the commit message next time. Thanks!

1st1 · 2018-01-28T22:28:48Z

Thanks, Victor!

1st1 requested review from vstinner and asvetlov December 19, 2017 14:24

bedevere-bot added the awaiting merge label Dec 19, 2017

the-knights-who-say-ni added the CLA signed label Dec 19, 2017

1st1 changed the title ~~Add socket.getblocking() method.~~ bpo-32373: Add socket.getblocking() method. Dec 19, 2017

1st1 force-pushed the getblocking branch from c4ab706 to d7a9dd3 Compare December 19, 2017 14:25

vstinner requested changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 19, 2017

python deleted a comment from bedevere-bot Dec 19, 2017

1st1 added awaiting review and removed awaiting changes labels Dec 19, 2017

vstinner previously approved these changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Dec 19, 2017

1st1 force-pushed the getblocking branch from 6da93d1 to 6599ba4 Compare December 19, 2017 15:22

asvetlov reviewed Dec 19, 2017

View reviewed changes

asvetlov approved these changes Dec 19, 2017

View reviewed changes

1st1 force-pushed the getblocking branch from d98d7bd to 35f3330 Compare December 19, 2017 19:49

vstinner reviewed Dec 19, 2017

View reviewed changes

1st1 force-pushed the getblocking branch 3 times, most recently from 3b8074e to 5f1cf87 Compare December 19, 2017 23:14

pitrou reviewed Dec 19, 2017

View reviewed changes

vstinner requested changes Dec 19, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 19, 2017

vstinner requested changes Dec 19, 2017

View reviewed changes

vstinner reviewed Dec 20, 2017

View reviewed changes

njsmith reviewed Dec 20, 2017

View reviewed changes

1st1 added 7 commits January 28, 2018 15:08

bpo-32373: Add socket.getblocking() method.

e411fc1

Address review.

c737e29

Fix testInitNonBlocking test

3e65a90

More setblocking/settimeout tests

8487f58

Add docs/comments

3a1898d

Trim down comments; remove low-level docs from socket.rst.

240af33

Clarify docs

4fac6d3

1st1 force-pushed the getblocking branch from f1b608a to 4fac6d3 Compare January 28, 2018 20:21

vstinner approved these changes Jan 28, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 28, 2018

vstinner reviewed Jan 28, 2018

View reviewed changes

1st1 merged commit f11b460 into python:master Jan 28, 2018

bedevere-bot removed the awaiting merge label Jan 28, 2018

bpo-32373: Add socket.getblocking() method. #4926

bpo-32373: Add socket.getblocking() method. #4926

Conversation

1st1 commented Dec 19, 2017 • edited by bedevere-bot Loading

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Dec 19, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Jan 28, 2018

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jan 28, 2018

1st1 commented Jan 28, 2018

1st1 commented Dec 19, 2017 •

edited by bedevere-bot

Loading

1st1 Dec 19, 2017 •

edited

Loading