New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Problem with SocketIO for closing the socket #48076
Comments
See example code attached. A very basic http server responds with an XML What's curious is that commenting out the single line "self.dummy = To reproduce:
Repeat with 2.5.2 - both variants work. I don't know much at all about python internals, but it seems that 1) a |
My initial description doesn't state the actual observable problem very Also forgot to mention that it's possible to work around this by |
Someone suggested I test this in 2.6rc1. The problem does not exist in |
3.0rc1 still fails. |
So the GC behaviour in this case is such that Python 3.0 can't collect |
The garbage collector does collect unreachable objects. What happens is that with python 2, the socket is explicitly closed by The cause is in the socket.makefile() function: since python3, the |
The whole socket._io_refs thing looks like a real design flaw. What is When close is called on the socket object itself, the socket MSUT be Its up to the programming to ensure that no other references to that I think this behavior started with this change: ------------------------------------------------------------------------ Make sure socket.close() doesn't interfere with socket.makefile(). If a makefile()-generated object is open and its parent socket is This change fixes httplib so that several urllib2net test cases pass Add SocketCloser class to socket.py, which encapsulates the Move SocketIO class from io module to socket module. It's only use is Add unittests to test_socket to cover various patterns of close and ............... ------------------------------------------------------------------------ Patch 1439 by Bill Janssen. I think this will work. |
On Thu, Sep 25, 2008 at 10:57 PM, Gregory P. Smith
The original purpose was to support the original semantics of the
See above -- the underlying connection must remain open until the Why do you say MUST? Is there an Internet standard that requires this?
No, the .makefile() API always promised that the connection remained
No, it's much, much older than that. That change was probably just a |
Alright I've taken another fresh look at this. I understand the dup Attached are two patches, either one of these will fix the problem and Personally I prefer the socket.py patch as I think it will solve this The socketserver.py patch also works but seems like more of a hack as it Please review. I'm guessing its too late in the release candidate process for 3.0 to |
P.S. Gabriel Genellina (gagenellina) - Your comment sounded like you |
I agree that the patch on socket.py is the correct fix: the raw socket I have one remark on the patch: |
Indeed IOBase does call close() from its __del__. Excellent. That |
Patch issue3826_socket-gps03.diff is OK for me. Someone should review both patches. |
I don't think this is quite right yet. If I run import socket
s=socket.socket()
s.connect(('www.python.org',80))
x=s.makefile()
del x
del s and put a print statement into real_close, I don't see that real_close |
But real_close() is not called either in the trivial socket use: import socket
s=socket.socket()
s.connect(('www.python.org',80))
del s OTOH, I added some printf statements in socketmodule.c, near all usages import socket
s=socket.socket()
s.connect(('www.python.org',80))
x=s.makefile()
x.close()
del s |
Martin: socket.socket has no destructor so a call to Amaury: The case you show doesn't call SOCKETCLOSE() because x still In order to fix that, SocketIO needs to drop its reference on close. |
gps05.diff includes the fix and unittests. |
--- El vie 28-nov-08, Gregory P. Smith <report@bugs.python.org> escribió:
I've found it; it uses BaseHTTPRequestHandler as in the original bug report. I'm not sure it is still relevant, but I'm attaching it here anyway. |
Issue bpo-4791 is exaclty the same problem (io reference in SocketIO): I |
Comment about issue3826_gps05.diff:
|
from #python-dev discussion. agreed, magic attributes are annoying. also, my gps05 patch continues I like your patch in bpo-4791 but lets keep both sets of our unit tests. Also, I think the SocketIO.fileno mathod should change to: def fileno(self):
no = self._sock.fileno()
if no == -1:
raise IOError("no file descriptor, socket is closed.") io.IOBase.fileno already raises IOError when no file descriptor is thoughts? |
Since SocketIO.fileno() just calls self._sock.fileno(), it would be |
I spent two hours on this issue and here are some notes... (1) SocketIO.close() have to call self._sock._decref_socketios() to s = socket...
f = s.makefile()
f.close() # only close the "file view"
s.close() <= close the socket here (2) SocketIO.close() have to break its reference to allow the socket s = socket...
f = s.makefile()
del s # there is still one reference (f._sock)
f.close() <= the garbage collector will close the socket here (3) operations on a closed SocketIO should be blocked s = socket...
f = s.makefile()
f.close()
f.fileno() => error! So issue3826_gps05.diff have two bugs:
|
socket_real_close-5.patch: my own patch developed for the issue bpo-4791
You may merge my patch with gps's patch (which has different tests). See also issue bpo-4853 to block all operations on closed socket in the |
Committed all of our tests and the actual code to fix the problem from This still needs back porting to release30-maint assuming no other |
How can we check if they are new issues with the changes? |
That mostly meant let the buildbots run it and/or see if anyone |
backported to release30-maint in r68796. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: