This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: New ver. of 1102879: Fix for 926423: socket timeouts
Type: Stage:
Components: Extension Modules Versions: Python 2.5
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: akuchling Nosy List: akuchling, nnorwitz, tony_nelson
Priority: release blocker Keywords: patch

Created on 2006-07-07 23:02 by tony_nelson, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
socketctlc.patch tony_nelson, 2006-07-07 23:02 Timeout Ctrl-C patch for Modules/socketmodule.c
socketctlc2.patch tony_nelson, 2006-07-24 01:58 Timeout Ctrl-C patch for Modules/socketmodule.c r50793
socketctlc4.patch tony_nelson, 2006-07-29 03:46 Timeout Ctrl-C patch with test for Modules/socketmodule.c r50921
socketctlc5.patch tony_nelson, 2006-07-29 21:56 Timeout Ctrl-C patch with *nix test for Modules/socketmodule.c r50959
socketctlc24a.patch tony_nelson, 2006-10-04 00:10 Timeout Ctrl-C patch with *nix test for Modules/socketmodule.c 2.4 r52122
Messages (11)
msg50627 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-07-07 23:02
This is a new version of the patch for 1102879, "Fix
for 926423: socket timeouts + Ctrl-C don't play nice".
 It passes "make EXTRATESTOPTS=-unetwork test".  I've
also made a version for Python 2.4.3 (for FC5), at
<http://georgeanelson.com/socketctlc24.patch>, which
also passes tests, and works with yum on FC5.

This is my first patch to Python.  I didn't see a way
to attach a file to the old patch.

My patch is slightly different from the original patch.
 My patch brings the error path back to the main line:
where the original patch would call
PyErr_SetFromErrno() for internal_select() errors, mine
initializes the status vars so that the normal error
handler will be called, and detects timeout when
internal_select() returns 1.  Thus the normal
(replaceable) error handler is called for errors
whether or not a timeout is set.

The patch handles connect() calls, and
sock_connect_ex() now checks for signals as would
PyErr_SetFromErrno().  I didn't change the name
"timeout" as it didn't bother me.  I didn't add any
confusing macros.

This patch took so long because of confusion (and
vacation).  Before the patch, Ctl-C would produce an
EWOULDBLOCK error, after the patch an EINTR error, but
still no KeyboardInterrupt exception.  It too me way
too long to notice that the rpm library used by yum
sets its own signal handlers, and that one line of code
(setting SIGINT back to the python default) would fix
it.  Now that I have KeyboardInterrupts I have
confidence in this patch.
msg50628 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-07-23 18:36
Logged In: YES 
user_id=33168

I'm +0.5 or more this going in to 2.5 final.  However, I'd
really like to see a test for this. Also, the patch should
be updated to HEAD.  It doesn't look like it will apply
cleanly as there were many changes to socketmodule.c.

As for a test, I think if you try to connect to a
non-existant host and send a signal from another thread, I
think that can trigger the bug.

Anthony, let me know your thoughts.
msg50629 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-07-24 01:58
Logged In: YES 
user_id=1356214

Thank you for your advice.  I have updated the patch for
HEAD (r50793).  It passes "make EXTRATESTOPTS=-unetwork
test" except for test_compile, which fails test_unary_minus.

Should I try to write the test?  I think that it could try
to accept() and another thread could send a signal.  I think
it would have to be separate from all the other tests, which
are threaded, so that it won't interfere with them.
msg50630 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-07-25 03:58
Logged In: YES 
user_id=33168

The test_unary_minus problem is due to gcc4.1 optimizations.
 We need to fix that.

Yes, please write a test.  As long as you can reproduce the
problem with the old code and the new code doesn't crash,
that's an improvement.  Thanks!

Not sure what you mean by "test".  It should at least be in
a separate method.  Perhaps you could use a new class in
test_socket.  I doubt that a new file should be necessary. 
Even a new class seems like it probably shouldn't be
necessary, but I can't say without actually having written
the test.  We'll figure out where to put it as long as we
have a test case.
msg50631 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-07-29 03:46
Logged In: YES 
user_id=1356214

Here is a new patch that adds a test, in test_socket.py
TCPTimeoutTest.testInterruptedTimeout(); the functional part
of the patch is unchanged.  The test doesn't use an actual
Ctl-C, or even a SIGINT, as sending one of them seemed hard;
it sends a SIGALRM instead.  The test passes with the patch;
without the patch it prints a confused error message, but
unfortunately that's about right.

As for what I mean by "test", well, I was confused about
what was being threaded.  I have it straight now.
msg50632 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-07-29 14:36
Logged In: YES 
user_id=1356214

Darn.  My test will only work on *nix, and not on other
platforms such as MSWindows.  I don't see a way to send a
signal that would work on all platforms, so I don't know a
way to test the patch.  I'll have to ask for help.  The test
may need to be changed to only test on *nix.
msg50633 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-07-29 18:23
Logged In: YES 
user_id=33168

Does the bug affect Windows?  A test that only runs on Unix
is better than nothing.  If the test can't be fixed to run
on Windows, please add a comment with an XXX to discuss this
and try to get someone to impl a Windows version.
msg50634 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-07-29 21:56
Logged In: YES 
user_id=1356214

I'm pretty sure that the bug would affect MSWindows.  I
don't know how to make the test work without sending a
signal, such as signal.alarm() or os.kill(), which don't
seem to be available on other platforms.  I've marked the
test with XXX and a comment as requested.  I've posed the
question on python-dev.  Should I ask there for specific
help writing the test?

The test in this version of the patch has fewer race
conditions (and reports the bug differently) than the
previous test.  The functional part of the patch is unchanged.
msg50635 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-08-02 06:48
Logged In: YES 
user_id=33168

Thanks!

Committed revision 51039. (2.5)
msg50636 - (view) Author: Tony Nelson (tony_nelson) Date: 2006-10-04 00:10
Logged In: YES 
user_id=1356214

Here is a version of the patch for the 2.4.4 maintenance branch.
msg50637 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-10-04 04:56
Logged In: YES 
user_id=33168

Assigning to Andrew if he's interested in backporting.
History
Date User Action Args
2022-04-11 14:56:18adminsetgithub: 43635
2006-07-07 23:02:41tony_nelsoncreate