Tinche / aiofiles

File support for asyncio
Apache License 2.0
2.67k stars 149 forks source link

remove readable from delegate_to_executor #28

Closed msiddorn closed 1 year ago

msiddorn commented 7 years ago

The AsyncTextIOWrapper has the attribute 'readable' in both the delegate_to_executor and the proxy_method_directly decorators. This causes the readable() method to be overwritten with a coroutine and creates a difference in behaviour between AsyncTextIOWrapper and the other async wrappers.

# open with TextIOBase
async with aiofiles.open('file.txt', 'r') as async_fo:
    # expect coroutine
    can_read = await async_fo.readable()

# open with BufferedReader
async with aiofiles.open('file.txt', 'rb') as async_fo:
    # expect function
    can_read = async_fo.readable()

Changes proposed remove 'readable' from delegate_to_executor decorator.

Tinche commented 7 years ago

Yeah there's something fishy going on here, since I see readable() is listed both in delegate_to_executor and proxy_method_directly. But the question is whether readable() is a blocking method or not?

msiddorn commented 7 years ago

The purpose of this PR was to bring the behaviour of AsyncTextIOWrapper in line with that of AsyncBufferedReader and AsyncFileIO, as I believe that readable() should be called in the same way from each. The underlying readable() function is the same for all three as it is commonly inherited from IOBase. However, it may be that the desired behaviour for all async wrappers is to have 'readable' in delegate_to_executor instead of proxy_method_directly, in which case the binary wrappers should be updated, this would also ensure that readable() and writable() (which is already a coroutine) are treated equally. I'm not sure whether these methods are blocking or not, are they directly querying a property of the IO objects? Either way the following are reasonable:

  1. The methods which are inherited from BaseIO should be called in the same way from all asynchronous wrappers.
  2. The readable() and writable() methods should be called in the same way. (At least I can't see a reason why not)
  3. 'readable' should not be duplicated in the decorators of AsyncTextIOWrapper
Tinche commented 1 year ago

Gonna close this as stale, hope that's ok ;)