Skip to content
This repository was archived by the owner on Jul 8, 2025. It is now read-only.

Conversation

argaen
Copy link

@argaen argaen commented Jun 5, 2019

No description provided.

"""
Check if there is n length data in the buffer
"""
if len(self._buffer) >= n:
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of comments here.

The final implementation of the read(n) [1] function always returns immediately if there is any data but having the data size bound to n. So theoretically the is_data_available should check only if there is something in the buffer. BTW this might explain why you are not seeing your code path executed in the acceptance test

Second, but TBH taking into account that this is a POC is not a stopper for me is the accessing of an internal attribute like _buffer that theoretically could be changed so breaking derivated classes. If we would go for a code that would have to be executed in production I won't rely on having always that attribute, so we will be forced to implement our own buffer attribute.

[1] https://github.com/python/cpython/blob/master/Lib/asyncio/streams.py#L1006

)
self.task_read.add_done_callback(self._read_cb)
if self.reader.is_data_available(length):
self._read_cb(self.reader.read(n=length))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a coro and a coro does not implement the Future interface, indeed the coro won't be scheduled in that scenario, not even started.

My suggestion is to implement a new method in the StreamReader for reading without having to wait and then if you want to wrap the result in a Future this will allow you to reuse the _read_cb function, for example

data = self.reader.read_nowait(n)
if data:
    f = Future()
    f.set_result(data)
    self._read_cb(f)
else:
    # awaitable version of the code

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants