Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: pd.ExcelFile closes stream on destruction #32544

1 change: 1 addition & 0 deletions doc/source/whatsnew/v1.0.2.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Fixed regressions
- Fixed regression in the repr of an object-dtype :class:`Index` with bools and missing values (:issue:`32146`)
- Fixed regression in :meth:`read_csv` in which the ``encoding`` option was not recognized with certain file-like objects (:issue:`31819`)
- Fixed regression in :meth:`DataFrame.reindex` and :meth:`Series.reindex` when reindexing with (tz-aware) index and ``method=nearest`` (:issue:`26683`)
- Fixed regression in :class:`ExcelFile` where the stream passed into the function was closed by the destructor. (:issue:`31467`)
- Fixed regression in :meth:`DataFrame.reindex_like` on a :class:`DataFrame` subclass raised an ``AssertionError`` (:issue:`31925`)
- Fixed regression in :meth:`Series.shift` with ``datetime64`` dtype when passing an integer ``fill_value`` (:issue:`32591`)

Expand Down
12 changes: 4 additions & 8 deletions pandas/io/excel/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,9 @@ def _workbook_class(self):
def load_workbook(self, filepath_or_buffer):
pass

def close(self):
TomAugspurger marked this conversation as resolved.
Show resolved Hide resolved
pass

@property
@abc.abstractmethod
def sheet_names(self):
Expand Down Expand Up @@ -895,14 +898,7 @@ def sheet_names(self):

def close(self):
"""close io if necessary"""
if self.engine == "openpyxl":
# https://stackoverflow.com/questions/31416842/
# openpyxl-does-not-close-excel-workbook-in-read-only-mode
wb = self.book
wb._archive.close()

if hasattr(self.io, "close"):
self.io.close()
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentionally removed? Wondering if there is any engine that needs this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bug fix

self._reader.close()

def __enter__(self):
return self
Expand Down
5 changes: 5 additions & 0 deletions pandas/io/excel/_openpyxl.py
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,11 @@ def load_workbook(self, filepath_or_buffer: FilePathOrBuffer):
filepath_or_buffer, read_only=True, data_only=True, keep_links=False
)

def close(self):
# https://stackoverflow.com/questions/31416842/
# openpyxl-does-not-close-excel-workbook-in-read-only-mode
self.book.close()

@property
def sheet_names(self) -> List[str]:
return self.book.sheetnames
Expand Down
19 changes: 15 additions & 4 deletions pandas/tests/io/excel/test_readers.py
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,17 @@ def test_read_from_py_localpath(self, read_ext):

tm.assert_frame_equal(expected, actual)

@td.check_file_leaks
def test_close_from_py_localpath(self, read_ext):

# GH31467
str_path = os.path.join("test1" + read_ext)
with open(str_path, "rb") as f:
x = pd.read_excel(f, "Sheet1", index_col=0)
del x
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to construct this test without the del call? This test might not be doing anything, since del doesn't really mean __del__ gets called

Copy link
Contributor Author

@roberthdevries roberthdevries Mar 11, 2020

Choose a reason for hiding this comment

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

__del__ will get called in practice when the last reference is deleted. This is needed to trigger the erroneous behavior.
This is literally the code in the OP's issue. (except I replaced ExcelFile with read_excel, it seems)

# should not throw an exception because the passed file was closed
f.read()

def test_reader_seconds(self, read_ext):
if pd.read_excel.keywords["engine"] == "pyxlsb":
pytest.xfail("Sheets containing datetimes not supported by pyxlsb")
Expand Down Expand Up @@ -1020,10 +1031,10 @@ def test_excel_read_buffer(self, engine, read_ext):
tm.assert_frame_equal(expected, actual)

def test_reader_closes_file(self, engine, read_ext):
f = open("test1" + read_ext, "rb")
with pd.ExcelFile(f) as xlsx:
# parses okay
pd.read_excel(xlsx, "Sheet1", index_col=0, engine=engine)
with open("test1" + read_ext, "rb") as f:
TomAugspurger marked this conversation as resolved.
Show resolved Hide resolved
with pd.ExcelFile(f) as xlsx:
# parses okay
pd.read_excel(xlsx, "Sheet1", index_col=0, engine=engine)

assert f.closed

Expand Down