Problem with SocketIO for closing the socket

[romkyns]

BPO	3826
Nosy	@loewis, @gpshead, @amauryfa, @vstinner
Files	breakage.py: Example reproducing the issue issue3826_gps05.diff: fix for socket.SocketIO and unit tests test_httpclose_py3k.py socket_real_close-5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2009-01-20.04:03:01.541>
created_at = <Date 2008-09-09.21:21:11.373>
labels = ['type-bug', 'library', 'release-blocker']
title = 'Problem with SocketIO for closing the socket'
updated_at = <Date 2009-01-20.04:03:01.513>
user = 'https://bugs.python.org/romkyns'

bugs.python.org fields:

activity = <Date 2009-01-20.04:03:01.513>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2009-01-20.04:03:01.541>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2008-09-09.21:21:11.373>
creator = 'romkyns'
dependencies = []
files = ['11448', '12165', '12166', '12615']
hgrepos = []
issue_num = 3826
keywords = ['patch']
message_count = 28.0
messages = ['72917', '72980', '73248', '73481', '73774', '73777', '73834', '73849', '76567', '76568', '76569', '76570', '76572', '76592', '76595', '76625', '76631', '76633', '79209', '79211', '79213', '79214', '79220', '79221', '79645', '79993', '79994', '80233']
nosy_count = 8.0
nosy_names = ['loewis', 'jhylton', 'gregory.p.smith', 'amaury.forgeotdarc', 'ggenellina', 'vstinner', 'zanella', 'romkyns']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue3826'
versions = ['Python 3.0']

[romkyns]

See example code attached. A very basic http server responds with an XML
document to any GET request. I think what happens is that the connection
remains open at the end of the request, leading the browser to believe
there's more data to come (and hence not displaying anything).

What's curious is that commenting out the single line "self.dummy =
self" fixes the problem. Also, the problem occurs in 3.0b2 but not in 2.5.2.

To reproduce:

1. Run the example code on 3.0b2
2. Navigate to http://localhost:8123/
   The browser shows "Transferring data" or something similar, until it
   times out.
3. Comment out the line "self.dummy = self"
4. Navigate to http://localhost:8123/
   This time it loads instantly.

Repeat with 2.5.2 - both variants work.

I don't know much at all about python internals, but it seems that 1) a
circular reference prevents the request handler from being GC'd and 2)
http.server relies on the request being GC'd to finalise the connection.

[romkyns]

My initial description doesn't state the actual observable problem very
clearly: the problem is that the browser gets stuck instead of
displaying the page, writing something along the lines of "Transferring
data from localhost". After many seconds it times out. Aborting the
request in Firefox causes the page to be displayed.

Also forgot to mention that it's possible to work around this by
explicitly removing all circular references after the base class'
__init__ method - so for the attached example a work-around is
"self.dummy = None" as the last line of the __init__ method.

[romkyns]

Someone suggested I test this in 2.6rc1. The problem does not exist in
2.6rc1, only in 3.0b2.

[ggenellina]

3.0rc1 still fails.
The diagnostic is correct, the connection should be closed after
sending the response, but isn't.
The attached unittest reproduces the error without requiring a browser.

[romkyns]

So the GC behaviour in this case is such that Python 3.0 can't collect
the object whereas Python 2.6 can. Is this known / expected or should
this be recorded in a separate issue?

[amauryfa]

The garbage collector does collect unreachable objects.

What happens is that with python 2, the socket is explicitly closed by
the HTTPServer, whereas with python 3, the explicit close() does not
work, and the socket is ultimately closed when the request has finished
and all objects are disposed.

The cause is in the socket.makefile() function: since python3, the
underlying socket uses a reference count, so that:
s = <a connected socket>
f = s.makefile()
s.close()
does not close the socket! adding f.close() is not enough. A "del f" is
necessary to really close the underlying socket.

[gpshead]

The whole socket._io_refs thing looks like a real design flaw. What is
its actual intended purpose?

When close is called on the socket object itself, the socket MSUT be
closed. Why is our API otherwise?

Its up to the programming to ensure that no other references to that
socket wrapped in whatever layers will be used after that, if they are
they'll eventually get errors when an IO occurs.

I think this behavior started with this change:

------------------------------------------------------------------------
r56714 | jeremy.hylton | 2007-08-03 13:40:09 -0700 (Fri, 03 Aug 2007) |
21 lines

Make sure socket.close() doesn't interfere with socket.makefile().

If a makefile()-generated object is open and its parent socket is
closed, the parent socket should remain open until the child is
closed, too. The code to support this is moderately complex and
requires one extra slots in the socket object.

This change fixes httplib so that several urllib2net test cases pass
again.

Add SocketCloser class to socket.py, which encapsulates the
refcounting logic for sockets after makefile() has been called.

Move SocketIO class from io module to socket module. It's only use is
to implement the raw I/O methods on top of a socket to support
makefile().

Add unittests to test_socket to cover various patterns of close and
makefile.

...............
later modified by this (still the same effect):
...............

------------------------------------------------------------------------
r58970 | guido.van.rossum | 2007-11-14 14:32:02 -0800 (Wed, 14 Nov 2007)
| 6 lines

Patch 1439 by Bill Janssen. I think this will work.
Tested on Windows by Christian Heimes.
I changed the code slightly, renaming decref_socketios() to
_decref_socketios(), and moving it closer to the close() method
that it calls. Hopefully Bill can now submit his SSL port to 3.0.

[gvanrossum]

On Thu, Sep 25, 2008 at 10:57 PM, Gregory P. Smith
<report@bugs.python.org> wrote:

The whole socket._io_refs thing looks like a real design flaw. What is
its actual intended purpose?

The original purpose was to support the original semantics of the
.makefile() method on Windows which doesn't have dup() (or at least
didn't have it when this hack was first invented, many years ago). We
almost got rid of it for Py3k, but then we realized that the same
issue (sort of) exists for ssl, and that's why it's still there and
used everywhere, not just on Windows.

When close is called on the socket object itself, the socket MSUT be
closed. Why is our API otherwise?

See above -- the underlying connection must remain open until the
stream created with .makefile() is also closed. (These are the
original dup-based semantics.)

Why do you say MUST? Is there an Internet standard that requires this?

Its up to the programming to ensure that no other references to that
socket wrapped in whatever layers will be used after that, if they are
they'll eventually get errors when an IO occurs.

No, the .makefile() API always promised that the connection remained
open until you closed (or lost) the stream.

I think this behavior started with this change:

------------------------------------------------------------------------
r56714 | jeremy.hylton | 2007-08-03 13:40:09 -0700 (Fri, 03 Aug 2007) |
21 lines

Make sure socket.close() doesn't interfere with socket.makefile().

No, it's much, much older than that. That change was probably just a
refactoring -- it's been refactored a number of times.

[gpshead]

Alright I've taken another fresh look at this. I understand the dup
semantics issue and don't want to break that.

Attached are two patches, either one of these will fix the problem and
breakage.py test code attached to this bug.

Personally I prefer the socket.py patch as I think it will solve this
problem in other places it could potentially pop up. It calls
self._sock._decref_socketios() from within the SocketIO.close() method
so that after a SocketIO returned by socket.makefile() is closed, it
will no longer prevent the underlying socket from closing.

The socketserver.py patch also works but seems like more of a hack as it
is still relies on immediate reference counted destruction.

Please review.

I'm guessing its too late in the release candidate process for 3.0 to
get this in, but we should do it for 3.0.1.

[gpshead]

P.S. Gabriel Genellina (gagenellina) - Your comment sounded like you
had a unit test for this but it never got attached. Still have it?

[amauryfa]

I agree that the patch on socket.py is the correct fix: the raw socket
should be detached when the close() method is called.

I have one remark on the patch:
io.IOBase.__del__ already calls close(): could SocketIO.__del__ be
removed completely? And no need for "_need_to_decref_sock": the closed
property should be enough.