PYTHON-2824 Make GridOut implement full io.IOBase spec #677
Conversation
There was a problem hiding this comment.
I've narrowed down the test failures to be caused by this change:
- class GridOut(object):
+ class GridOut(io.IOBase):
Reverting this line causes the test_retryable_reads tests to pass again but I have no idea why. This needs more investigation.
The other test failure (test_collection.TestCollection.test_manual_last_error) will be fixed if you rebase your changes using git pull --rebase upstream master.
…erited methods. Make GridOut inherit from io.IOBase to be a fully "file-like" object (https://docs.python.org/3/glossary.html#term-file-like-object). Implement missing methods `readlines`, `writelines`, `writable`, `fileno`, `flush`, `isatty`, `truncate`, and property `closed`, following the spec (https://docs.python.org/3/library/io.html#io.IOBase.writable).
Add test asserting exceptions / false for `writelines`, `writable`, `fileno`, `flush`, `isatty`, `truncate`. Add unittests for `readlines` and `closed`.
… iterator conform to IOBase interface GridOut does not need its own `__closed` attribute and property `closed`, as base class IOBase already provides that and thus, a call to `super().close()` in GridOut `close` suffices. GridOut `readlines` is also already implemented in IOBase using `readline`, so GridOut does not need its own implementation. Iterating over GridOut previously returned chunks, but IOBase specifies that lines should be returned. Thus, the `GridOutIterator` returning chunks is removed and GridOut simply uses the existing IOBase iterator implementation (returning `self` in `__iter__` and using `readline` in `__next__`). Additionally, iterating over GridOut previously did not move the "file pointer" along, i.e. `next(iter(some_grid_out_object))` always gave the same result (the first chunk of the file) as it would create a new iterator starting at the top of the file. This is now fixed as well, so a first call to `next(iter(some_grid_out_object))` gives the first line, and subsequent calls return the subsequent lines. The test_grid_file.py `test_iterator` is changed accordingly.
henrifroese
force-pushed
the
PYTHON-2824/grid-out-implement-iobase-interface
branch
from
a266c94 to
1a5a0f4
Compare
|
Interesting; I've made the other changes and will now have a look at the tests. (As an aside, it took some time to get the |
|
So the first failure (test
Overriding def __del__(self):
passmakes all tests pass. As far as I can see, we don't need the destructor - there're no system resources or anything we want to release come garbage collection |
|
One possibility would now be to just override the destructor to |
|
Great find! I am still confused as to why
We could but this would be yet another subtle breaking change and I'd like to avoid adding more of these. |
|
Actually not sure about that. Minimal example to reproduce would be this: >>> import io
>>> class A(io.IOBase):
... def __getattr__(self, item):
... print(f"__getattr__ called with {item=}")
...
>>>
>>> a = A()
>>> a.closed
__getattr__ called with item='__IOBase_closed'
True
>>>
>>> del a
__getattr__ called with item='__IOBase_closed' |
|
Thanks @henrifroese, apologies for the delay. My plan is to create a Python bug report for the IOBase.__closed + getattr issue but we don't need to hold up this PR for it. I suggest going with your override # Override IOBase.__del__ otherwise it will lead to __getattr__ on
# __IOBase_closed which calls _ensure_file and potentially performs I/O.
# We cannot do I/O in __del__ since it can lead to a deadlock.
def __del__(self):
pass |
IOBase `__del__` calls `__getattr__` on `__IOBase_closed` which calls `_ensure_file and potentially performs I/O. We cannot do I/O in `__del__` since it can lead to a deadlock. It is thus overridden to just `pass`.
|
Makes sense, done 👌 |
|
Thanks @henrifroese! I opened https://jira.mongodb.org/browse/PYTHON-2874 to make sure GridIn also implements io.IOBase. |
No description provided.