Arvados

FWIW, the python built-in docs for file object state that readlines() repeatedly calls readline(), so perhaps a fix is to reimplement it that way rather than via readall().

fc308658aeeeb007fa3eb710b4bdc9179a13db10

branch 7852-test-readlines

Added a test that fails when realine and readlines are invoked on the stream reader in that sequence. The readlines call is only fetching the data after the "first block" which was read during the first readline call. It appears that self._readline_cache and self._filepos are playing a role here.

d1c1f71d3d2e4170c51855d23557d58322627fd8

Added one more test "test_readline_then_readall" which is failing exactly like the previous readline_the_readlines test, for apparently the same reason.

Should we reset self._readline_cache and self._filepos when a readall or a readlines call is made? Looking at the python api documentation, it seems like the readall and readlines calls are expected to read the entire file contents.

https://docs.python.org/2/library/io.html

7852-readline-cache-fix-wip is up for review. It starts from Radhika's 7852-test-readlines branch, rebases that on top of current master, then adds a commit that fixes the bug and gets the tests passing.

Since this bug is about the interaction between readline() and readlines(), I'm looking at the branch now and readlines() doesn't actually use readline(). Worse, it looks like readlines() sucks in the entire file into a buffer before using splitlines() to return a list. I suggest refactoring readlines() to be an iterator over readline().

Peter Amstutz wrote:

Since this bug is about the interaction between readline() and readlines(), I'm looking at the branch now and readlines() doesn't actually use readline().

That's how the bug was reported, but it's worth noting that the bug will strike any time a caller interleaves calls to readline() and any other file read method. This is demonstrated by Radhika's readall test, and noted in the commit message.

Worse, it looks like readlines() sucks in the entire file into a buffer before using splitlines() to return a list. I suggest refactoring readlines() to be an iterator over readline().

readlines() is a standard file method that returns a list, so changing that to an iterator would break the API. Obviously calling it in any non-trivial case is a Bad Idea, but that's for the caller to decide, not us.

Given that API, sucking in whole blocks actually improves the performance of the method, because it avoids the overhead of calling readline() many times when the Keep API currently requires us to fetch whole blocks anyway.