classification
Title: socket.sendall() crash when receiving a signal
Type: crash Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: exarkun, gregory.p.smith, pitrou
Priority: normal Keywords: patch

Created on 2010-09-25 19:07 by pitrou, last changed 2010-09-28 07:44 by ned.deily. This issue is now closed.

Files
File name Uploaded Description Edit
sendallinterrupt.patch pitrou, 2010-09-25 20:27
Messages (8)
msg117385 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 19:07
This was introduced by r74426 which addressed issue1628205. socket.sendall() calls PyErr_CheckSignals() (and potentially returns to the caller) without having the GIL.

>>> import socket
>>> c, s = socket.socketpair()
>>> s.sendall(b"x"*(100 * 1024**2))
^C^CFatal Python error: PyThreadState_Get: no current thread
msg117387 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 19:11
The fix is very simple, but perhaps a test should be added.

diff -r af0d7b32d6ce Modules/socketmodule.c
--- a/Modules/socketmodule.c    Fri Sep 24 20:03:12 2010 +0200
+++ b/Modules/socketmodule.c    Sat Sep 25 21:09:58 2010 +0200
@@ -2581,8 +2581,8 @@ sock_sendall(PySocketSockObject *s, PyOb
         return select_error();
     }
 
-    Py_BEGIN_ALLOW_THREADS
     do {
+        Py_BEGIN_ALLOW_THREADS
         timeout = internal_select(s, 1);
         n = -1;
         if (timeout)
@@ -2592,6 +2592,7 @@ sock_sendall(PySocketSockObject *s, PyOb
 #else
         n = send(s->sock_fd, buf, len, flags);
 #endif
+        Py_END_ALLOW_THREADS
         if (n < 0) {
 #ifdef EINTR
             /* We must handle EINTR here as there is no way for
@@ -2610,7 +2611,6 @@ sock_sendall(PySocketSockObject *s, PyOb
         buf += n;
         len -= n;
     } while (len > 0);
-    Py_END_ALLOW_THREADS
     PyBuffer_Release(&pbuf);
 
     if (timeout == 1) {
msg117388 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 19:26
Actually, the patch is enough to suppress the crash, but sendall() behaviour is buggy in another way: EINTR may be received as part of the select() call (on sockets with a timeout), in which case the loop will be exited early instead of retrying, losing track of the number of bytes written (something which the original patch aimed to avoid).

For example:

>>> def handler(*args): print (args)
... 
>>> signal.signal(signal.SIGALRM, handler)
0
>>> c, s = socket.socketpair()
>>> s.settimeout(60.0)
>>> signal.alarm(1); s.sendall(b"x" * (100 * 1024**2))
0
(14, <frame object at 0x2b8f220>)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
socket.error: [Errno 4] Interrupted system call
msg117389 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 19:54
Oh, and an additional bug is that send() can return a successful partial write when it was actually interrupted by a signal.
msg117390 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-25 20:27
Here is a patch fixing the aforementioned issues, and with tests.
msg117453 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-27 17:56
Patch committed in r85032. I'm gonna watch the buildbots a bit, in case the test fails on some platforms.
msg117460 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-27 18:33
The patch passes at least on Linux, OS X and Solaris buildbots. Backported to 3.1 (r85034) and 2.7 (r85035).
msg117505 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-09-28 05:59
thanks Antoine!
History
Date User Action Args
2010-09-28 07:44:59ned.deilysetfiles: - unnamed
2010-09-28 05:59:12gregory.p.smithsetfiles: + unnamed

messages: + msg117505
2010-09-27 18:33:38pitrousetstatus: open -> closed

messages: + msg117460
2010-09-27 17:56:37pitrousetassignee: pitrou
resolution: fixed
messages: + msg117453
stage: resolved
2010-09-25 20:27:25pitrousetfiles: + sendallinterrupt.patch

nosy: + exarkun
messages: + msg117390

keywords: + patch
2010-09-25 19:54:22pitrousetmessages: + msg117389
2010-09-25 19:26:20pitrousetmessages: + msg117388
2010-09-25 19:11:19pitrousetmessages: + msg117387
2010-09-25 19:07:46pitroucreate