Issue2470

classification
Title: Jython leaks sockets in select reactor
Type: Severity: critical
Components: Core Versions: Jython 2.7
Milestone: Jython 2.7.1
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zyasoft Nosy List: nickmbailey, zyasoft
Priority: high Keywords:

Created on 2016-02-19.20:52:01 by nickmbailey, last changed 2016-03-14.16:41:24 by zyasoft.

Files
File name Uploaded Description Edit Remove
reactorbug.tar.gz nickmbailey, 2016-02-19.20:52:00
Messages
msg10759 (view) Author: Nick (nickmbailey) Date: 2016-02-19.20:52:00
Jython's implementation of sockets/select is causing socket's to leak inside the twisted select reactor implementation.

I've uploaded a tarball with a script that demonstrates the problem. If you run './runjython.sh' you will see 10 http requests made and never cleaned up in the reactor. Running './runpython.sh' will demonstrate the same problem doesn't exist in python. 

I've tried to do some analysis of what the issue is on my own but some input from some people with more experience will be helpful.

I think what is happening is:

* We open a socket
* After successful connection, we add a callback to handle the underlying netty channel being closed: https://github.com/jythontools/jython/blob/master/Lib/_socket.py#L878
* After the http request, the netty channel is closed, our callback runs, and adds the PEER_CLOSED message to the sockets incoming buffer
* The _writable method of _realsocket depends on the channel being active and open: https://github.com/jythontools/jython/blob/master/Lib/_socket.py#L1123
* So after the netty channel closes, _writable will always return false. This means that select will never return this socket object as writable again and it will sit in a half closed state forever.

So I *think* the right solution is some additional logic in '_writable'. The underlying netty channel being closed doesn't necessarily mean we are down with that socket.
msg10760 (view) Author: Nick (nickmbailey) Date: 2016-02-19.21:04:32
The following patch fixes the issue for me by setting 'self.peer_closed' earlier and returning True from '_writable' if self.peer_closed is True. I'm not 100% positive that's the correct thing to do in all cases though.

diff --git a/Lib/_socket.py b/Lib/_socket.py
index 3b658e5..f1987d5 100644
--- a/Lib/_socket.py
+++ b/Lib/_socket.py
@@ -877,6 +877,7 @@ class _realsocket(object):

         def peer_closed(x):
             log.debug("Peer closed channel %s", x, extra={"sock": self})
+            self.peer_closed = True
             self.incoming.put(_PEER_CLOSED)
             self._notify_selectors(hangup=True)

@@ -1121,7 +1122,7 @@ class _realsocket(object):
         return 0

     def _writable(self):
-        return self.channel and self.channel.isActive() and self.channel.isWritable()
+        return self.peer_closed or (self.channel and self.channel.isActive() and self.channel.isWritable())

     can_write = _writable

@@ -1176,7 +1177,6 @@ class _realsocket(object):
         if msg is _PEER_CLOSED:
             # Only return _PEER_CLOSED once
             self.incoming_head = None
-            self.peer_closed = True
         return msg

     @raises_java_exception
msg10765 (view) Author: Nick (nickmbailey) Date: 2016-02-23.23:22:51
Jim pointed out that the initial diff I included breaks other tests. I modified it slightly and managed to get my example working without breaking any other tests.

diff --git a/Lib/_socket.py b/Lib/_socket.py
index 3b658e5..4b3c7df 100644
--- a/Lib/_socket.py
+++ b/Lib/_socket.py
@@ -729,6 +729,7 @@ class _realsocket(object):
         self.selectors = CopyOnWriteArrayList()
         self.options = {}  # deferred options until bootstrap
         self.peer_closed = False
+        self.channel_closed = False

         # Reference count this underlying socket
         self.open_lock = Lock()
@@ -875,12 +876,13 @@ class _realsocket(object):
         if self.connect_handlers:
             self.channel.pipeline().addLast(self.python_inbound_handler)

-        def peer_closed(x):
+        def channel_closed(x):
             log.debug("Peer closed channel %s", x, extra={"sock": self})
+            self.channel_closed = True
             self.incoming.put(_PEER_CLOSED)
             self._notify_selectors(hangup=True)

-        self.channel.closeFuture().addListener(peer_closed)
+        self.channel.closeFuture().addListener(channel_closed)

     def connect(self, addr):
         # Unwrapped sockets can immediately perform the post-connect step
@@ -1121,7 +1123,7 @@ class _realsocket(object):
         return 0

     def _writable(self):
-        return self.channel and self.channel.isActive() and self.channel.isWritable()
+        return self.channel_closed or (self.channel and self.channel.isActive() and self.channel.isWritable())

     can_write = _writable


I'd still like the change to be vetted though.
msg10770 (view) Author: Jim Baker (zyasoft) Date: 2016-02-24.05:57:15
Fixed as of https://hg.python.org/jython/rev/63a2a4f6aa42
History
Date User Action Args
2016-03-14 16:41:24zyasoftsetstatus: pending -> closed
2016-02-24 06:29:30zyasoftsetresolution: accepted -> fixed
2016-02-24 05:57:15zyasoftsetstatus: open -> pending
messages: + msg10770
2016-02-24 00:08:03zyasoftsetpriority: high
2016-02-23 23:22:52nickmbaileysetmessages: + msg10765
2016-02-21 02:28:41zyasoftsetassignee: zyasoft
resolution: accepted
nosy: + zyasoft
milestone: Jython 2.7.1
2016-02-19 21:04:33nickmbaileysetmessages: + msg10760
2016-02-19 20:52:01nickmbaileycreate