[Patches] [ python-Patches-1519025 ] New ver. of 1102879: Fix for 926423: socket timeouts

Wed Aug 2 08:48:18 CEST 2006

Patches item #1519025, was opened at 2006-07-07 16:02
Message generated for change (Settings changed) made by nnorwitz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1519025&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Modules
>Group: Python 2.5
>Status: Closed
>Resolution: Accepted
Priority: 8
Submitted By: Tony Nelson (tony_nelson)
>Assigned to: Neal Norwitz (nnorwitz)
Summary: New ver. of 1102879: Fix for 926423: socket timeouts

Initial Comment:
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.

----------------------------------------------------------------------

>Comment By: Neal Norwitz (nnorwitz)
Date: 2006-08-01 23:48

Message:
Logged In: YES 
user_id=33168

Thanks!

Committed revision 51039. (2.5)

----------------------------------------------------------------------

Comment By: Tony Nelson (tony_nelson)
Date: 2006-07-29 14:56

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-29 11:23

Message:
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.

----------------------------------------------------------------------

Comment By: Tony Nelson (tony_nelson)
Date: 2006-07-29 07:36

Message:
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.

----------------------------------------------------------------------

Comment By: Tony Nelson (tony_nelson)
Date: 2006-07-28 20:46

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-24 20:58

Message:
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.

----------------------------------------------------------------------

Comment By: Tony Nelson (tony_nelson)
Date: 2006-07-23 18:58

Message:
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.

----------------------------------------------------------------------

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-07-23 11:36

Message:
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.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1519025&group_id=5470