Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
225 changes: 187 additions & 38 deletions asyncio/streams.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
__all__ = ['StreamReader', 'StreamWriter', 'StreamReaderProtocol',
'open_connection', 'start_server',
'IncompleteReadError',
'LimitOverrunError',
]

import socket
Expand All @@ -27,15 +28,28 @@ class IncompleteReadError(EOFError):
Incomplete read error. Attributes:

- partial: read bytes string before the end of stream was reached
- expected: total number of expected bytes
- expected: total number of expected bytes (or None if unknown)
"""
def __init__(self, partial, expected):
EOFError.__init__(self, "%s bytes read on a total of %s expected bytes"
% (len(partial), expected))
super().__init__("%d bytes read on a total of %r expected bytes"
% (len(partial), expected))
self.partial = partial
self.expected = expected


class LimitOverrunError(Exception):
"""Reached buffer limit while looking for the separator.

Attributes:
- message: error message
- consumed: total number of bytes that should be consumed
"""
def __init__(self, message, consumed):
super().__init__(message)
self.message = message
self.consumed = consumed


@coroutine
def open_connection(host=None, port=None, *,
loop=None, limit=_DEFAULT_LIMIT, **kwds):
Expand Down Expand Up @@ -318,6 +332,10 @@ class StreamReader:
def __init__(self, limit=_DEFAULT_LIMIT, loop=None):
# The line length limit is a security feature;
# it also doubles as half the buffer limit.

if limit <= 0:
raise ValueError('Limit cannot be <= 0')

self._limit = limit
if loop is None:
self._loop = events.get_event_loop()
Expand Down Expand Up @@ -361,7 +379,7 @@ def set_exception(self, exc):
waiter.set_exception(exc)

def _wakeup_waiter(self):
"""Wakeup read() or readline() function waiting for data or EOF."""
"""Wakeup read*() functions waiting for data or EOF."""
waiter = self._waiter
if waiter is not None:
self._waiter = None
Expand Down Expand Up @@ -409,7 +427,10 @@ def feed_data(self, data):

@coroutine
def _wait_for_data(self, func_name):
"""Wait until feed_data() or feed_eof() is called."""
"""Wait until feed_data() or feed_eof() is called.

If stream was paused, automatically resume it.
"""
# StreamReader uses a future to link the protocol feed_data() method
# to a read coroutine. Running two read coroutines at the same time
# would have an unexpected behaviour. It would not possible to know
Expand All @@ -418,6 +439,13 @@ def _wait_for_data(self, func_name):
raise RuntimeError('%s() called while another coroutine is '
'already waiting for incoming data' % func_name)

assert not self._eof, '_wait_for_data after EOF'

# Waiting for data while paused will make deadlock, so prevent it.
if self._paused:
self._paused = False
self._transport.resume_reading()

self._waiter = futures.Future(loop=self._loop)
try:
yield from self._waiter
Expand All @@ -426,43 +454,150 @@ def _wait_for_data(self, func_name):

@coroutine
def readline(self):
"""Read chunk of data from the stream until newline (b'\n') is found.

On success, return chunk that ends with newline. If only partial
line can be read due to EOF, return incomplete line without
terminating newline. When EOF was reached while no bytes read, empty
bytes object is returned.

If limit is reached, ValueError will be raised. In that case, if
newline was found, complete line including newline will be removed
from internal buffer. Else, internal buffer will be cleared. Limit is
compared against part of the line without newline.

If stream was paused, this function will automatically resume it if
needed.
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opening """ should be on the same line as the first word of the docstring.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

sep = b'\n'
seplen = len(sep)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need sep and seplen variables, just use b'\n' and 1 in the code. That will actually improve the readability.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, it was. But I'm intentionally give names to that values, since it is not obvious what is 1 for example.

Anyways, if @gvanrossum approve, I will fix.

try:
line = yield from self.readuntil(sep)
except IncompleteReadError as e:
return e.partial
except LimitOverrunError as e:
if self._buffer.startswith(sep, e.consumed):
del self._buffer[:e.consumed + seplen]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining what's going on here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a comment, saying that this restore original behaviour of readline (see - I even did not touch original tests). Since "original behaviour" is unknown for people seeing that comment in future, I decide to remove comment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment(s) should be restored. The only reason we have these two except clauses is for backwards compatibility, so we should state that. Otherwise in the future people may waste time trying to understand the reason or try to fix it and accidentally break code that depends on the old way. (Actually, the return e.partial is for compatibility with readline() methods on synchronous I/O classes; the backwards compatibility concern applies to deleting the consumed portion of the buffer only.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Alternatively, in the future people may legitimately decide to leave the excess data in the buffer for the LimitOverrunError case -- it helps that the only reason to not do that was backwards compatibility, not anything deeper.)

else:
self._buffer.clear()
self._maybe_resume_transport()
raise ValueError(e.args[0])
return line

@coroutine
def readuntil(self, separator=b'\n'):
"""Read chunk of data from the stream until `separator` is found.

On success, chunk and its separator will be removed from internal buffer
(i.e. consumed). Returned chunk will include separator at the end.

Configured stream limit is used to check result. Limit means maximal
length of chunk that can be returned, not counting the separator.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this blank line.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

If EOF occurs and complete separator still not found,
IncompleteReadError(<partial data>, None) will be raised and internal
buffer becomes empty. This partial data may contain a partial separator.

If chunk cannot be read due to overlimit, LimitOverrunError will be raised
and data will be left in internal buffer, so it can be read again, in
some different way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum @socketpair Why do we leave the data in the buffer? Suppose one implements a MIME parser, and tries to read until they see a boundary. Would it make sense for them to handle the LimitOverrunError, and instead of closing the connection, trying to recover with just reading the data? Is there a realistic use case for this?

The reason I'm asking this, is because I don't like that readline and readuntil will behave differently.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In previous implementation there was configurable limit. now it has gone. So, user may make a warning, and try to read with bigger limit, or, read by portions to skip that bad data...or, maybe read until another separator, trying to recover from error.

One of the most unwanted feature of readline - is to leave stream in unpredicatable state when such error occur.

I can remove that odd (as you think) behaviour, but in that case I should add closing of connection right before raising such exception. (at least, disable any readings and simulate EOF).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[..] and try to read with bigger limit, or, read by portions to skip that bad data...or, maybe read until another separator, trying to recover from error.

Unfortunately I haven't seen such an advanced code yet. Usually the code just crashes, and the connection gets closed :)

One of the most unwanted feature of readline - is to leave stream in unpredicatable state when such error occur.

Agree. But... The readline method before your PR did exactly that? Sorry, I'm a bit confused now.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, readline before me just clears buffer (sometimes, depending on fact it found newline or not), and was not closing connection. You may see original, not so obvious code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, even though it is an obscure border case. There is actually an alternative way to read that buffered data -- using plain read().


If stream was paused, this function will automatically resume it if
needed.
"""
seplen = len(separator)
if seplen == 0:
raise ValueError('Separator should be at least one-byte string')

if self._exception is not None:
raise self._exception

line = bytearray()
not_enough = True

while not_enough:
while self._buffer and not_enough:
ichar = self._buffer.find(b'\n')
if ichar < 0:
line.extend(self._buffer)
self._buffer.clear()
else:
ichar += 1
line.extend(self._buffer[:ichar])
del self._buffer[:ichar]
not_enough = False

if len(line) > self._limit:
self._maybe_resume_transport()
raise ValueError('Line is too long')
# Consume whole buffer except last bytes, which length is

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@socketpair It's hard for me to match your comments here with the code below.

Could you please split this huge comment into smaller ones and move them closer to the relevant pieces of code?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot split, since it comments single line of code. I can move it down and split by newline(s) inside comment. OK?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me leave some comments below with suggestions on how we can annotate the lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are welcome. I have some problems with my English, since it is not native language for me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are welcome. I have some problems with my English, since it is not native language for me.

NP, it's not my native language either. I'm mostly asking for comments because I've troubles following the code, so I want to have almost every significant line of it annotated.

# one less than seplen. Let's check corner cases with
# separator='SEPARATOR':
# * we have received almost complete separator (without last
# byte). i.e buffer='some textSEPARATO'. In this case we
# can safely consume len(separator) - 1 bytes.
# * last byte of buffer is first byte of separator, i.e.
# buffer='abcdefghijklmnopqrS'. We may safely consume
# everything except that last byte, but this require to
# analyze bytes of buffer that match partial separator.
# This is slow and/or require FSM. For this case our
# implementation is not optimal, since require rescanning
# of data that is known to not belong to separator. In
# real world, separator will not be so long to notice
# performance problems. Even when reading MIME-encoded
# messages :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# `offset` is the number of bytes from the beginning of the buffer
# where is no occurrence of `separator`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

# `offset` is the number of bytes from the beginning of the buffer where
# is no occurrence of `separator`.
offset = 0

# Loop until we find `separator` in the buffer, exceed the buffer size,
# or an EOF has happened.
while True:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Loop until we find `separator` in the buffer, exceed the buffer size,
# or an EOF has happened.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

buflen = len(self._buffer)

# Check if we now have enough data in the buffer for `separator` to
# fit.
if buflen - offset >= seplen:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Check if we now have enough data in the buffer for `separator` to fit.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

isep = self._buffer.find(separator, offset)

if isep != -1:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# `separator` is in the buffer. `isep` will be used later to
# retrieve the data.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

# `separator` is in the buffer. `isep` will be used later to
# retrieve the data.
break

# see upper comment for explanation.
offset = buflen + 1 - seplen

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know what, instead of having the lengthy comment explaining the "+1" part here, maybe we can just have

offset = buflen - seplen

? It's way easier to understand (although I now follow your logic), and there is no way you can see any performance difference.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, should it be

offset = max(buflen - seplen, 0)

?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the rest of the review to Yury -- I trust him completely (even
his English -- note that English isn't my mother tongue either :-).

On Mon, Dec 14, 2015 at 1:55 PM, Yury Selivanov notifications@github.com
wrote:

In asyncio/streams.py
#297 (comment):

  •    #   This is slow and/or require FSM. For this case our
    
  •    #   implementation is not optimal, since require rescanning
    
  •    #   of data that is known to not belong to separator. In
    
  •    #   real world, separator will not be so long to notice
    
  •    #   performance problems. Even when reading MIME-encoded
    
  •    #   messages :)
    
  •    offset = 0
    
  •    while True:
    
  •        buflen = len(self._buffer)
    
  •        if buflen - offset >= seplen:
    
  •            isep = self._buffer.find(separator, offset)
    
  •            if isep != -1:
    
  •                break
    
  •            # see upper comment for explanation
    
  •            offset = buflen + 1 - seplen
    

BTW, should it be

offset = max(buflen - seplen, 0)

?


Reply to this email directly or view it on GitHub
https://github.com/python/asyncio/pull/297/files#r47564276.

--Guido van Rossum (python.org/~guido)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Algorythms should be clear, I have checked all corner cases. So I can just remove long comment, but prefer strict rules.
  2. Maybe some day I implement Boyer Moore algorithm or something more appropriate (like KMP), which also allows to detect LimitOverrunError earlier on some data/separator combinations.
  3. Please provide case when max(buflen - seplen, 0) is required (I will add it to tests if required)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Algorythms should be clear, I have checked all corner cases. So I can just remove long comment, but prefer strict rules.

That was your checking, and it's your preference. However, I had to do my own checking, and other asyncio devs will do theirs, for as long as your code exists. Reading your huge comment and studying all corner cases consumes a lot of our time for no actual performance gains.

Maybe some day I implement Boyer Moore algorithm or something more appropriate (like KMP), which also allows to detect LimitOverrunError earlier on some data/separator combinations.

Feel free to make a PR when this day comes :)

Please provide case when max(buflen - seplen, 0) is required (I will add it to tests if required)

You're right, we don't need it since the code is guarded with if buflen - offset >= seplen:.

if offset > self._limit:
raise LimitOverrunError('Separator is not found, and chunk exceed the limit', offset)

# Complete message (with full separator) may be present in buffer
# even when EOF flag is set. This may happen when the last chunk
# adds data which makes separator be found. That's why we check for
# EOF *ater* inspecting the buffer.
if self._eof:
break
chunk = bytes(self._buffer)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this if clause be the first if in this while loop?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, since we may have complete record in buffer + EOF flag triggered. In that case we should return complete record without any exceptions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I suggest adding a comment about this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

self._buffer.clear()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should not clear the buffer here? The same argument applies -- there's another way to read the rejected data if an app really wants it. That may be better than stuffing it in the exception.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gvanrossum

Maybe we should not clear the buffer here?

  1. It is compatibility with readexactly.
  2. If LimitOverrun occurs, this may not mean some protocol violation (say, in HTTP there are no limits on length in various places). This exception is not "fatal", and we should preserve data in order to continue. For example, users of library may issue warning in logs and try to re-read with bigger limit. UPD: Specifying limit in that function was removed (but I do not agree with that, but remove it in order to be merged).
  3. If IncompleteReadError occurs, this mean protocol violation (or sudden disconnection). That mean requested operation can never complete. Yes, someone may want to re-read data in different way, but I can not provide real use case (as opposed to LimitOverrun case). Partially fetched record can not be used in a general, so this exception is "fatal". If someone wants to inspect what data still have been fetched - it have ability to. If I remove buffer cleanup, and user wants to get that partial data, he will required to call .read() in a loop in order to fetch everything unread, since we can not get amount of data in buffer (and call readexactly). Moreover, it is common to use this function to read data line-by-line, and so users should distinguish between empty and non-empty data after this exception happen. In other words, having data inside THIS exception is very suitable.

raise IncompleteReadError(chunk, None)

# _wait_for_data() will resume reading if stream was paused.
yield from self._wait_for_data('readuntil')

if not_enough:
yield from self._wait_for_data('readline')
if isep > self._limit:
raise LimitOverrunError('Separator is found, but chunk is longer than limit', isep)

chunk = self._buffer[:isep + seplen]
del self._buffer[:isep + seplen]
self._maybe_resume_transport()
return bytes(line)
return bytes(chunk)

@coroutine
def read(self, n=-1):
"""Read up to `n` bytes from the stream.

If n is not provided, or set to -1, read until EOF and return all read
bytes. If the EOF was received and the internal buffer is empty, return
an empty bytes object.

If n is zero, return empty bytes object immediatelly.

If n is positive, this function try to read `n` bytes, and may return
less or equal bytes than requested, but at least one byte. If EOF was
received before any byte is read, this function returns empty byte
object.

Returned value is not limited with limit, configured at stream creation.

If stream was paused, this function will automatically resume it if
needed.
"""

if self._exception is not None:
raise self._exception

if not n:
if n == 0:
return b''

if n < 0:
Expand All @@ -477,29 +612,41 @@ def read(self, n=-1):
break
blocks.append(block)
return b''.join(blocks)
else:
if not self._buffer and not self._eof:
yield from self._wait_for_data('read')

if n < 0 or len(self._buffer) <= n:
data = bytes(self._buffer)
self._buffer.clear()
else:
# n > 0 and len(self._buffer) > n
data = bytes(self._buffer[:n])
del self._buffer[:n]
if not self._buffer and not self._eof:
yield from self._wait_for_data('read')

# This will work right even if buffer is less than n bytes
data = bytes(self._buffer[:n])
del self._buffer[:n]

self._maybe_resume_transport()
return data

@coroutine
def readexactly(self, n):
"""Read exactly `n` bytes.

Raise an `IncompleteReadError` if EOF is reached before `n` bytes can be
read. The `IncompleteReadError.partial` attribute of the exception will
contain the partial read bytes.

if n is zero, return empty bytes object.

Returned value is not limited with limit, configured at stream creation.

If stream was paused, this function will automatically resume it if
needed.
"""
if n < 0:
raise ValueError('readexactly size can not be less than zero')

if self._exception is not None:
raise self._exception

if n == 0:
return b''

# There used to be "optimized" code here. It created its own
# Future and waited until self._buffer had at least the n
# bytes, then called read(n). Unfortunately, this could pause
Expand All @@ -516,6 +663,8 @@ def readexactly(self, n):
blocks.append(block)
n -= len(block)

assert n == 0

return b''.join(blocks)

if compat.PY35:
Expand Down
Loading