-
-
Notifications
You must be signed in to change notification settings - Fork 182
Add new StreamReader.readuntil() method. #297
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| __all__ = ['StreamReader', 'StreamWriter', 'StreamReaderProtocol', | ||
| 'open_connection', 'start_server', | ||
| 'IncompleteReadError', | ||
| 'LimitOverrunError', | ||
| ] | ||
|
|
||
| import socket | ||
|
|
@@ -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): | ||
|
|
@@ -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() | ||
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -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. | ||
| """ | ||
| sep = b'\n' | ||
| seplen = len(sep) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment explaining what's going on here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete this blank line.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The reason I'm asking this, is because I don't like that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately I haven't seen such an advanced code yet. Usually the code just crashes, and the connection gets closed :)
Agree. But... The
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 :) | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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`.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| isep = self._buffer.find(separator, offset) | ||
|
|
||
| if isep != -1: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, should it be offset = max(buflen - seplen, 0)?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 On Mon, Dec 14, 2015 at 1:55 PM, Yury Selivanov notifications@github.com
--Guido van Rossum (python.org/~guido)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Feel free to make a PR when this day comes :)
You're right, we don't need it since the code is guarded with |
||
| 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. I suggest adding a comment about this.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
| self._buffer.clear() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| 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: | ||
|
|
@@ -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 | ||
|
|
@@ -516,6 +663,8 @@ def readexactly(self, n): | |
| blocks.append(block) | ||
| n -= len(block) | ||
|
|
||
| assert n == 0 | ||
|
|
||
| return b''.join(blocks) | ||
|
|
||
| if compat.PY35: | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed