msg65472 - (view) |
Author: Curt Hagenlocher (CurtHagenlocher) |
Date: 2008-04-14 18:30 |
First reported by Ralf Schmitt.
I haven't checked 2.6 or 3.0.
|
msg65473 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-04-14 18:33 |
The relevant python-dev thread is
http://mail.python.org/pipermail/python-dev/2008-April/078613.html
|
msg65485 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-14 21:49 |
One more time: the change is wrong. It should try to recv the maximum
not the minimum of size, buffer_size. If you using a buffering of 16
bytes it will otherwise call recv 256 times when you want to read 1024
bytes. this is wrong.
However there should be an upper limit, it doesn't make sense to read
10MB from socket in one recv call (imap bug).
|
msg65486 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-14 21:51 |
akuchling, I added you to the nosy list. hope that's ok.
|
msg65487 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-04-14 22:10 |
This is apparently the same issue as #2601.
|
msg65489 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-14 22:59 |
ahh. yes, same issue. should have taken a look at the bugtracker before,
would have saved me some time...
|
msg65499 - (view) |
Author: Curt Hagenlocher (CurtHagenlocher) |
Date: 2008-04-15 05:24 |
I've attached a much better patch as suggested by Guido
|
msg65500 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-15 05:42 |
This patch handles the case where the caller has specified the size
argument. When size is unspecified, it should be handled as if size was
infinite. By the formula from your patch, this should be
recv_size = min(self.max_readsize, max(self._rbufsize, left))
(== min(self.max_readsize, inf) == self.max_readsize)
and not the current code:
if self._rbufsize <= 1:
recv_size = self.default_bufsize
else:
recv_size = self._rbufsize
while True:
data = self._sock.recv(recv_size)
BTW, I still think this max_readsize limit should be handled somewhere
deeper in the code. at least maybe in the _socketobject class.
|
msg65528 - (view) |
Author: Curt Hagenlocher (CurtHagenlocher) |
Date: 2008-04-15 20:45 |
At least in 2.5.2, there appear to be only two places in the standard
library where an arbitrary and unchecked size is passed directly to
_socketobject.recv. _fileobject.read is one such place and the other
is in logging/config.py. In both cases, the pattern is something like
while len(data) < size:
data = data + sock.recv(size - len(data))
Of course, the same pattern may exist in any number of instances of
user code that performs a socket.recv, and all of these would be
subject to the same memory thrashing problem that originally beset
imaplib.
As such, I now agree with Ralf that the fix ultimately belongs in
_socketobject. However, deploying a fix for 2.5.2 is probably easier
if it's in socket.py than if it requires a recompile.
|
msg65561 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-04-16 18:27 |
As A.M. Kuchling said in the other bug :), there is no need to fix 2.5.2
as the offending change is posterior to the release.
By the way, somewhat really ought to close either this bug or #2601,
discussing things in parallel in two different bug entries is too confusing.
|
msg65607 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-18 09:26 |
However it should be fixed in release25-maint. Can anyone install curt's
patch? It does what the original fix intended to do.
|
msg65664 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-04-21 18:00 |
Twisted fixed their problem for issue 1092502 by making recv()ed data
short lived by putting it into a StringIO for buffering.
I've attached a patch that does that for the socket module -and- gets
rid of any possibility of doing tiny size recvs in read() which is the
cause of the poor performance that this issue was filed about.
My patch also documents what _rbufsize is used for (readline) and makes
sane use of it with read.
When viewing the change, it is best to view it side by side next to the
old version rather than trying to comprehend it as a diff.
I believe this really fixes both issue 1092502 while at the same time
prevents introduction of a speed problem (this issue).
|
msg65669 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-04-22 06:02 |
(I think the twisted issue your talking about is:
http://twistedmatrix.com/trac/ticket/1079)
Your patch still contains this code:
recv_size = min(rbufsize, left)
data = self._sock.recv(recv_size)
This is IMHO wrong. See my comments above.
|
msg65756 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-04-25 00:41 |
available for an easy side by side review here:
http://codereview.appspot.com/212
Also, yes I think you're right Ralf. With these changes I should be
able to return that to a max() within the while True: for sized reads
and things will work as desired.
|
msg66098 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-05-02 16:40 |
I fixed the min vs max use that Ralf noted and have submitted this as
r62627. all tests pass for me but I believe it deserves a wider
audience and testing outside of just the test suite.
could those who reported the original problems (both the memory use
issue and the speed issue) run their tests with this applied?
... watching the buildbots ...
http://codereview.appspot.com/621/diff/61/81
|
msg66267 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-05-05 12:47 |
I have a case which triggers the assert in readline:
File "build/bdist.linux-x86_64/egg/setuptools/command/develop.py",
line 27, in run
File "build/bdist.linux-x86_64/egg/setuptools/command/develop.py",
line 102, in install_for_development
File
"build/bdist.linux-x86_64/egg/setuptools/command/easy_install.py", line
519, in process_distribution
File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 522, in resolve
File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 758, in
best_match
File "build/bdist.linux-x86_64/egg/pkg_resources.py", line 770, in obtain
File
"build/bdist.linux-x86_64/egg/setuptools/command/easy_install.py", line
433, in easy_install
File "build/bdist.linux-x86_64/egg/setuptools/package_index.py", line
462, in fetch_distribution
File "build/bdist.linux-x86_64/egg/setuptools/package_index.py", line
303, in find_packages
File "build/bdist.linux-x86_64/egg/setuptools/package_index.py", line
617, in scan_url
File "build/bdist.linux-x86_64/egg/setuptools/package_index.py", line
198, in process_url
File "/home/ralf/py26/lib/python2.6/socket.py", line 326, in read
data = self._sock.recv(rbufsize)
File "/home/ralf/py26/lib/python2.6/httplib.py", line 512, in read
return self._read_chunked(amt)
File "/home/ralf/py26/lib/python2.6/httplib.py", line 548, in
_read_chunked
line = self.fp.readline()
File "/home/ralf/py26/lib/python2.6/socket.py", line 391, in readline
assert buf.tell() == 0
AssertionError
this happens when I run :
easy_install 'mwlib<0.6'
|
msg66269 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-05-05 13:59 |
Similar problem is reported in #2760.
|
msg66278 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-05-05 19:20 |
I'm glad i put that assert in... The problem occurs due to a mixture of
fixed size reads followed by unbounded readlines on an unbuffered
_fileobject. A case that the code currently doesn't handle. I'm fixing it.
|
msg66294 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-05-05 21:54 |
The bug introduced in r62627 has been fixed in r62744.
|
msg66306 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-05-06 08:23 |
the above easy_install now works for me. thanks.
|
msg66400 - (view) |
Author: Barry A. Warsaw (barry) * |
Date: 2008-05-08 03:46 |
Bumping this down to critical for the alpha release. It should be
release blocker for the first beta.
|
msg66984 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-05-17 07:14 |
I'm going to let the committed change bake in Python trunk for a beta
release or two before backporting it to release25-maint.
|
msg67945 - (view) |
Author: Ralf Schmitt (schmir) |
Date: 2008-06-11 09:36 |
Can we get the fix for release25-maint? It will not get worse than the
current state is.
|
msg68191 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-06-13 23:46 |
If it's fixed in 2.6 and 3.0, it shouldn't be release blocker anymore,
now should it?
|
msg68192 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-06-13 23:46 |
Well, it's a release blocker for 2.5.3.
|
msg69315 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2008-07-06 04:04 |
committed to release25-maint in r64754.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:33 | admin | set | github: 46884 |
2008-07-06 04:04:51 | gregory.p.smith | set | status: open -> closed resolution: fixed messages:
+ msg69315 |
2008-06-13 23:46:55 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages:
+ msg68192 |
2008-06-13 23:46:03 | pitrou | set | messages:
+ msg68191 |
2008-06-11 09:36:27 | schmir | set | messages:
+ msg67945 |
2008-06-11 05:44:29 | gregory.p.smith | set | versions:
- Python 2.6 |
2008-05-17 07:14:54 | gregory.p.smith | set | messages:
+ msg66984 |
2008-05-14 22:42:11 | georg.brandl | set | priority: critical -> release blocker |
2008-05-08 03:46:36 | barry | set | priority: release blocker -> critical nosy:
+ barry messages:
+ msg66400 |
2008-05-06 08:23:31 | schmir | set | messages:
+ msg66306 |
2008-05-05 21:54:34 | gregory.p.smith | set | messages:
+ msg66294 |
2008-05-05 19:20:59 | gregory.p.smith | set | messages:
+ msg66278 |
2008-05-05 18:35:43 | gregory.p.smith | link | issue2760 superseder |
2008-05-05 18:35:43 | gregory.p.smith | link | issue2760 dependencies |
2008-05-05 13:59:50 | draghuram | set | messages:
+ msg66269 |
2008-05-05 12:47:27 | schmir | set | messages:
+ msg66267 |
2008-05-02 17:08:13 | gregory.p.smith | set | priority: critical -> release blocker |
2008-05-02 16:40:05 | gregory.p.smith | set | messages:
+ msg66098 |
2008-04-30 08:56:36 | doko | set | nosy:
+ doko |
2008-04-30 05:56:00 | mhammond | link | issue2601 superseder |
2008-04-25 00:41:13 | gregory.p.smith | set | messages:
+ msg65756 |
2008-04-22 06:02:36 | schmir | set | messages:
+ msg65669 |
2008-04-21 22:32:55 | forest | set | nosy:
+ forest |
2008-04-21 18:00:53 | gregory.p.smith | set | files:
+ socket-strio-gps01.patch.txt assignee: gregory.p.smith messages:
+ msg65664 nosy:
+ gregory.p.smith |
2008-04-18 09:26:23 | schmir | set | messages:
+ msg65607 |
2008-04-16 18:27:57 | pitrou | set | messages:
+ msg65561 |
2008-04-15 20:45:15 | CurtHagenlocher | set | messages:
+ msg65528 title: socket._fileobject.read(n) should ignore _rbufsize when 1 -> performance problem in socket._fileobject.read |
2008-04-15 05:42:16 | schmir | set | messages:
+ msg65500 |
2008-04-15 05:24:32 | CurtHagenlocher | set | files:
- socket.py.diff |
2008-04-15 05:24:23 | CurtHagenlocher | set | files:
+ socket.py.diff messages:
+ msg65499 |
2008-04-14 22:59:39 | schmir | set | messages:
+ msg65489 |
2008-04-14 22:10:24 | pitrou | set | nosy:
+ pitrou messages:
+ msg65487 |
2008-04-14 21:52:18 | schmir | set | nosy:
+ akuchling |
2008-04-14 21:51:30 | schmir | set | messages:
+ msg65486 |
2008-04-14 21:51:03 | gvanrossum | set | priority: critical |
2008-04-14 21:50:00 | schmir | set | versions:
+ Python 2.6 |
2008-04-14 21:49:44 | schmir | set | messages:
+ msg65485 |
2008-04-14 21:42:00 | schmir | set | nosy:
+ schmir |
2008-04-14 18:33:25 | draghuram | set | nosy:
+ draghuram messages:
+ msg65473 |
2008-04-14 18:30:16 | CurtHagenlocher | create | |