classification
Title: Fix BufferedRWPair
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.1
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: bquinlan, pitrou
Priority: release blocker Keywords: patch

Created on 2009-04-11 10:41 by bquinlan, last changed 2009-04-19 00:11 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
rwpair.diff bquinlan, 2009-04-11 10:41 Minimal working version of BufferedRWPair - tests are still not great
rwpair2.diff bquinlan, 2009-04-18 07:41 With fixes based on review
Messages (8)
msg85849 - (view) Author: Brian Quinlan (bquinlan) * (Python committer) Date: 2009-04-11 10:41
The C implementation:
- doesn't correctly initialize its reader and writer instances
- incorrectly maps its "readinto" method to another class
- incorrectly delegates its "closed" property to its base class

i.e. this class can't be used at all

The Python implementation:
- Calls internal methods of its constructor arguments that aren't
  part of the IOBase interface to determine if its streams are 
  readable/writable

There aren't any useful tests for either.
msg85850 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-11 10:46
Hey, thanks a lot for caring. I'll take a look at the patch and come
back later.
If you want to add some more tests, you are welcome too :-)
msg85870 - (view) Author: Brian Quinlan (bquinlan) * (Python committer) Date: 2009-04-11 17:50
Hey Antoine,

Will do - but first I'll finish up my reason for needing a working
version of this class in the first place ;-)

Cheers,
Brian
msg86090 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-17 20:59
I've uploaded the patch for code review here:
http://codereview.appspot.com/40126
msg86091 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-17 21:11
Reviewers: report_bugs.python.org,

Message:
Here are some comments. In general, I think the changes to _pyio.py are
unwarranted (even though I dislike the naming of _checkReadable and
_checkWritable).

http://codereview.appspot.com/40126/diff/1/2
File Lib/_pyio.py (left):

http://codereview.appspot.com/40126/diff/1/2#oldcode370
Line 370: def _checkReadable(self, msg=None):
Not sure why you're removing it. Currently it's used in Lib/socket.py.

http://codereview.appspot.com/40126/diff/1/2#oldcode384
Line 384: def _checkWritable(self, msg=None):
Same question as for _checkReadable().

http://codereview.appspot.com/40126/diff/1/3
File Lib/test/test_io.py (right):

http://codereview.appspot.com/40126/diff/1/3#newcode1121
Line 1121: self.assertTrue(pair.readable)
This is probably `pair.readable()` and not `pair.readable`.

http://codereview.appspot.com/40126/diff/1/3#newcode1125
Line 1125: self.assertTrue(pair.writable)
Same comment as for readable above.

http://codereview.appspot.com/40126/diff/1/3#newcode1126
Line 1126:
There should probably be a test for seekable() as well.

http://codereview.appspot.com/40126/diff/1/4
File Modules/_io/bufferedio.c (right):

http://codereview.appspot.com/40126/diff/1/4#newcode1876
Line 1876: Py_DECREF(self->reader);
You must use Py_CLEAR so that there is no double free when calling
BufferedRWPair_dealloc().

Please review this at http://codereview.appspot.com/40126

Affected files:
   M     Lib/_pyio.py
   M     Lib/test/test_io.py
   M     Modules/_io/bufferedio.c
msg86107 - (view) Author: Brian Quinlan (bquinlan) * (Python committer) Date: 2009-04-18 07:41
http://codereview.appspot.com/40126/diff/1/2
File Lib/_pyio.py (left):

http://codereview.appspot.com/40126/diff/1/2#oldcode370
Line 370: def _checkReadable(self, msg=None):
On 2009/04/17 21:11:15, Antoine Pitrou wrote:
> Not sure why you're removing it. Currently it's used in Lib/socket.py.

I didn't see the other usages. I removed it because it was only used
twice in this file and one of the usages involved an instance other than
self i.e. calling an internal method on another instance. Ditto for
_checkWriteable

Now restored.

http://codereview.appspot.com/40126/diff/1/3
File Lib/test/test_io.py (right):

http://codereview.appspot.com/40126/diff/1/3#newcode1121
Line 1121: self.assertTrue(pair.readable)
On 2009/04/17 21:11:15, Antoine Pitrou wrote:
> This is probably `pair.readable()` and not `pair.readable`.

Done.

http://codereview.appspot.com/40126/diff/1/3#newcode1125
Line 1125: self.assertTrue(pair.writable)
On 2009/04/17 21:11:15, Antoine Pitrou wrote:
> Same comment as for readable above.

Done.

http://codereview.appspot.com/40126/diff/1/3#newcode1126
Line 1126:
On 2009/04/17 21:11:15, Antoine Pitrou wrote:
> There should probably be a test for seekable() as well.

Now you are getting greedy. Done.

http://codereview.appspot.com/40126/diff/1/4
File Modules/_io/bufferedio.c (right):

http://codereview.appspot.com/40126/diff/1/4#newcode1876
Line 1876: Py_DECREF(self->reader);
On 2009/04/17 21:11:15, Antoine Pitrou wrote:
> You must use Py_CLEAR so that there is no double free when calling
> BufferedRWPair_dealloc().

Done.

http://codereview.appspot.com/40126
msg86150 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-18 23:55
The patch looks ok, thanks.
msg86151 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-19 00:11
Committed in r71736.
History
Date User Action Args
2009-04-19 00:11:21pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg86151
2009-04-18 23:55:16pitrousetresolution: accepted
messages: + msg86150
stage: patch review -> commit review
2009-04-18 07:41:33bquinlansetmessages: + msg86107
2009-04-18 07:41:15bquinlansetfiles: + rwpair2.diff
2009-04-17 21:11:17pitrousetmessages: + msg86091
title: BufferedRWPair broken -> Fix BufferedRWPair
2009-04-17 20:59:04pitrousetmessages: + msg86090
2009-04-11 17:50:51bquinlansetmessages: + msg85870
2009-04-11 10:46:54pitrousetpriority: release blocker

nosy: + pitrou
messages: + msg85850

assignee: pitrou
stage: patch review
2009-04-11 10:41:43bquinlansettype: behavior
components: + Library (Lib)
2009-04-11 10:41:08bquinlancreate