Issue2470
Created on 2016-02-19.20:52:01 by nickmbailey, last changed 2016-03-14.16:41:24 by zyasoft.
File name |
Uploaded |
Description |
Edit |
Remove |
reactorbug.tar.gz
|
nickmbailey,
2016-02-19.20:52:00
|
|
|
|
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
|
|
Date |
User |
Action |
Args |
2016-03-14 16:41:24 | zyasoft | set | status: pending -> closed |
2016-02-24 06:29:30 | zyasoft | set | resolution: accepted -> fixed |
2016-02-24 05:57:15 | zyasoft | set | status: open -> pending messages:
+ msg10770 |
2016-02-24 00:08:03 | zyasoft | set | priority: high |
2016-02-23 23:22:52 | nickmbailey | set | messages:
+ msg10765 |
2016-02-21 02:28:41 | zyasoft | set | assignee: zyasoft resolution: accepted nosy:
+ zyasoft milestone: Jython 2.7.1 |
2016-02-19 21:04:33 | nickmbailey | set | messages:
+ msg10760 |
2016-02-19 20:52:01 | nickmbailey | create | |
|