classification
Title: io.BufferedReader.peek(): Documentation differs from Implementation
Type: behavior Stage:
Components: Documentation, Library (Lib) Versions: Python 3.2, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: docs@python Nosy List: benjamin.peterson, conf, docs@python, martin.panter, ncoghlan, pitrou, trott
Priority: normal Keywords: patch

Created on 2009-04-22 06:15 by trott, last changed 2015-01-30 06:48 by martin.panter.

Files
File name Uploaded Description Edit
peek.diff conf, 2009-06-13 04:20
peek2.diff conf, 2009-06-13 04:23
peek3.diff conf, 2009-06-15 05:21
peek4.diff conf, 2009-06-15 13:02
peek-one-byte.patch martin.panter, 2015-01-10 04:26 Document ≥ 1 byte returned review
Messages (25)
msg86274 - (view) Author: Torsten Rottmann (trott) Date: 2009-04-22 06:15
The documented behavior of io.BufferedReader.peek([n]) states:

peek([n])
Return 1 (or n if specified) bytes from a buffer without advancing the 
position.

Thereas the parameter n is the _max_ length of returned bytes.

Implementation is:

    def _peek_unlocked(self, n=0):
        want = min(n, self.buffer_size)
        have = len(self._read_buf) - self._read_pos
        if have < want:
            to_read = self.buffer_size - have
            current = self.raw.read(to_read)
            if current:
                self._read_buf = self._read_buf[self._read_pos:] + 
current
                self._read_pos = 0
        return self._read_buf[self._read_pos:]

Where you may see that the parameter n is the _min_ length
of returned bytes. So peek(1) will return _not_ just 1 Byte, but the
remaining bytes in the buffer.
msg86275 - (view) Author: Torsten Rottmann (trott) Date: 2009-04-22 06:20
Note: this is also in Python 2.6
msg86276 - (view) Author: Torsten Rottmann (trott) Date: 2009-04-22 06:29
Proposed patch to fix this:

set the default of n to 1 as stated by docs:

def _peek_unlocked(self, n=1):

return n bytes:

return self._read_buf[self._read_pos:self._read_pos+n]
msg89311 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-06-13 02:27
Assigned to Benjamin for assessment - this should be considered for rc2
since it's still broken in 3.1:

>>> f = open('setup.py', 'rb')
>>> len(f.peek(10))
4096
>>> len(f.peek(1))
4096
>>> len(f.peek(4095))
4096
>>> len(f.peek(10095))
4096


Brought up on python-dev in this thread:
http://mail.python.org/pipermail/python-dev/2009-June/089986.html

And previously here:
http://mail.python.org/pipermail/python-dev/2009-April/088229.html

The thread from April suggests the current behaviour may be intentional,
in which case it is the documentation that needs to be fixed, as it is
currently not just misleading but flat out wrong. Then again, Benjamin's
initial response to that thread was to support the idea of changing
peek() so that the argument actually was a cap.

The previous documentation that Alexandre quotes in the April was
changed to the current description in late April without any
corresponding change to the implementation:
http://svn.python.org/view/python/branches/py3k/Doc/library/io.rst?r1=62422&r2=62430

However, the old description was also wrong for the io-c implementation
since it just returns the current buffered data from peek, no matter
what argument you pass in.
msg89312 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-06-13 03:38
I think the argument should be used as a upper bound; I will look at
this tomorrow.
msg89313 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-13 04:20
Hey guys, I did a patch about this one.
I didn't do many tests but I guess it is ok (it works like I think it
should).
What do you think?
msg89314 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-13 04:23
Oops I overlooked I minor flaw.
A second version.
msg89315 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-13 04:48
There's a problem with my patch... When the size of the data we want to
peek is too big ( > buffer_len - start ) the cursor will move, thus
there isn't a case where the peek function would work properly (except
when we want to peek() just 1 byte).
Couldn't we use a read() followed by a seek() instead?
msg89324 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-06-13 12:21
Lucas, it is indeed impossible for peek() to return more than the buffer
size and remain compatible with non-seekable raw streams. That's why it
/never/ returns more than the buffer size.

As for the fact that peek() doesn't behave as documented, I disagree.
Here is what the docstring says:

        """Returns buffered bytes without advancing the position.

        The argument indicates a desired minimal number of bytes; we
        do at most one raw read to satisfy it.  We never return more
        than self.buffer_size.
        """

Please note : "a desired /minimal/ number of bytes" (minimal, not
maximal). Furthermore, "We never return more than self.buffer_size." The
behaviour looks ok to me.
msg89326 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-13 12:37
We could fill the buffer while moving its start point to 0. I guess this
behavior would require a new function (or a new parameter to
Modules/_io/bufferedio.c:_bufferedreader_fill_buffer() ).
If you are ok with that I could write a patch.
msg89327 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-06-13 12:40
We could, however, enforce the passed argument and only return the whole
remaining buffer when no argument is given. This is a bit like Frederick
Reeve's proposal on python-dev, but less sophisticated and therefore
less tedious to implement.

In any case, I'm not sure it should be committed before the 3.1 release.
The second and last release candidate is supposed to be today.
msg89328 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-06-13 12:47
> We could fill the buffer while moving its start point to 0. I guess this
> behavior would require a new function (or a new parameter to
> Modules/_io/bufferedio.c:_bufferedreader_fill_buffer() ).
> If you are ok with that I could write a patch.

The buffer is used for both reading and writing and you have to be
careful when shifting it. Besides, the same change (or similar) should
also be done in the Python implementation (in _pyio.py).

If you come up with a patch, please add some tests and check the whole
regression suite passes.
msg89329 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-06-13 12:57
I'm downgrading this because it can't be changed until after 3.1 is
released.
msg89340 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-06-14 01:12
It's not the docstring that is wrong for the current behaviour, it's the
IO.BufferedReader documentation:

"""
peek([n])
    Return 1 (or n if specified) bytes from a buffer without advancing
the position. Only a single read on the raw stream is done to satisfy
the call. The number of bytes returned may be less than requested since
at most all the buffer’s bytes from the current position to the end are
returned.
"""

That gives absolutely no indication that the call might return more
bytes than expected, and the indication that leaving out the argument
will return only the next byte is flat out wrong.
msg89349 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-06-14 14:38
I updated the documentation in r73429. Is that better?
msg89366 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-06-14 19:32
Rather than "only a single read on the raw stream", it should be "at
most a single read on the raw stream", IMHO.
msg89388 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-15 05:21
Here is a patch that passes all the tests (I had to change some of them
though, they were expecting erroneous behaviours IMHO).
The biggest problem was the read1 testing, I've tried to get the maximum
of bytes less than or equal to what the user wanted while executing at
most 1 raw_read()'s.

I have created a new test for peek()'ing a number of bytes bigger than
could possibly be stored on the buffer.
msg89389 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-15 05:22
Here, it's a patch that passes all the tests (I had to change some of them
though, they were expecting erroneous behaviours IMHO).
The biggest problem was the read1 testing, I've tried to get the maximum
of bytes less than or equal to what the user wanted while executing at
most 1 raw_read()'s.

I have created a new test for peek()'ing a number of bytes bigger than
could possibly be stored on the buffer.
msg89399 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-06-15 10:37
I haven't read the patch in detail but I don't think you should have
changed read1(). read1() is there for optimization purposes and its
current behaviour makes sense IMO.
msg89400 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2009-06-15 10:39
The doc revision definitely does a better job of characterising the
current underspecified behaviour :)

I agree with Antoine that "at most a single read" would be better wording.
msg89401 - (view) Author: Lucas Prado Melo (conf) Date: 2009-06-15 13:02
Ok
A new patch without read1() changes.
Only one test fails, a read1() test:

======================================================================
FAIL: test_read1 (test.test_io.PyBufferedRWPairTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/lucas/Codes/python-stuff/py3k/Lib/test/test_io.py", line
1139, in test_read1
    self.assertEqual(pair.read1(3), b"abc")
AssertionError: b'a' != b'abc'


Since I've changed peek_unlocked() (which is used once by read1()), I
guess there's a problem with read1() expectations about it.
msg189599 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-05-19 15:19
Looks like this has slipped under the radar.  I'll leave working on it to the experts :)
msg233750 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-01-09 12:59
Is the current documentation as accurate as it can be?

“The number of bytes returned may be less or more than requested”

To me this has always made this method practically useless. A valid implementation could just always return b"". I noticed the BZ2File.peek() documentation (BZ2File is apparently trying to imitate BufferedReader) is slightly more useful:

“At least one byte of data will be returned (unless at EOF)”

That could be used for (say) peeking for a LF following a CR. But still the “size” parameter does not seem very useful. In fact, LZMAFile.peek() says the size is ignored.
msg233801 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-01-10 04:26
Here is a simple documentation patch to guarantee that at least one byte is normally returned. This would make the method much more useful, and compatible with the BZ2File and LZMAFile interfaces, allowing them to use BufferedReader, as I propose to do in Issue 15955.

Even if nobody is interested in Torsten’s patch to limit the return length, I suggest my patch be considered :)
msg235024 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-01-30 06:48
The non-blocking behaviour that I documented in my patch is under question in Issue 13322. I think it would be nice change the implementation to either return None or raise BlockingIOError.
History
Date User Action Args
2015-01-30 06:48:55martin.pantersetmessages: + msg235024
2015-01-10 04:26:53martin.pantersetfiles: + peek-one-byte.patch

nosy: + docs@python
messages: + msg233801

assignee: docs@python
components: + Documentation
2015-01-09 12:59:20martin.pantersetnosy: + martin.panter

messages: + msg233750
versions: + Python 3.4
2014-02-03 18:29:40BreamoreBoysetnosy: - BreamoreBoy
2013-05-19 15:19:52BreamoreBoysetnosy: + BreamoreBoy
messages: + msg189599
2009-06-15 13:02:29confsetfiles: + peek4.diff

messages: + msg89401
2009-06-15 10:39:12ncoghlansetmessages: + msg89400
2009-06-15 10:37:07pitrousetmessages: + msg89399
versions: + Python 2.7
2009-06-15 05:22:02confsetmessages: + msg89389
2009-06-15 05:21:10confsetfiles: + peek3.diff

messages: + msg89388
2009-06-14 19:32:54pitrousetmessages: + msg89366
2009-06-14 14:38:07benjamin.petersonsetmessages: + msg89349
versions: + Python 3.2, - Python 3.0
2009-06-14 01:12:27ncoghlansetmessages: + msg89340
2009-06-13 12:57:05benjamin.petersonsetpriority: release blocker -> normal
assignee: benjamin.peterson -> (no value)
messages: + msg89329
2009-06-13 12:47:34pitrousetmessages: + msg89328
2009-06-13 12:40:17pitrousetmessages: + msg89327
2009-06-13 12:37:37confsetmessages: + msg89326
2009-06-13 12:21:50pitrousetnosy: + pitrou
messages: + msg89324
2009-06-13 04:48:45confsetmessages: + msg89315
2009-06-13 04:23:11confsetfiles: + peek2.diff

messages: + msg89314
2009-06-13 04:20:57confsetfiles: + peek.diff

nosy: + conf
messages: + msg89313

keywords: + patch
2009-06-13 03:38:35benjamin.petersonsetmessages: + msg89312
2009-06-13 02:27:13ncoghlansetpriority: release blocker

nosy: + ncoghlan, benjamin.peterson
messages: + msg89311

assignee: benjamin.peterson
2009-04-22 06:29:14trottsetmessages: + msg86276
2009-04-22 06:20:09trottsetmessages: + msg86275
2009-04-22 06:15:52trottcreate