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: file leak in read_excel #30096

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Conversation

jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Dec 5, 2019

I think this
closes #30031
closes #29514
we'll probably want to wait a few days to make it official.

To make the new test decorator useful in the CI we'll need to add psutil to the environment (unless someone knows of a stdlib way of doing the same check?)

One more note, the check in check_no_file_leaks depends on __del__ having been called, which in turn depends on the gc. I'm not sure, but that might induce another layer of flakiness. If so, I think that can be fixed by calling gc.collect() before doing the flist == flist2 check

@alimcmaster1
Copy link
Member

LGTM - if this works! thanks for the fix!

@jreback jreback added the IO Excel read_excel, to_excel label Dec 5, 2019
@jreback jreback added this to the 1.0 milestone Dec 5, 2019
@jreback
Copy link
Contributor

jreback commented Dec 5, 2019

great. maybe keep rebasing a few times this to see if it fails.

@jbrockmendel
Copy link
Member Author

maybe keep rebasing a few times this to see if it fails.

Seems like a more effective way of checking this against many runs would be to merge and see if the py36 failures go away?

@jreback
Copy link
Contributor

jreback commented Dec 6, 2019

maybe keep rebasing a few times this to see if it fails.

Seems like a more effective way of checking this against many runs would be to merge and see if the py36 failures go away?

sure

@jonashaag
Copy link
Contributor

@jbrockmendel xref #47656: Does this miss a gc.collect() before the leak check? Some leaks will go away with a GC run. In #47656 there are a bunch of boundmethod objects that form a cycle with their originator class (SAS7BDATReader).

@jbrockmendel
Copy link
Member Author

That's plausible, but I'll need a lot more caffeine to say more than that. An extra better-safe-than-sorry gc.collect call wouldn't be the end of the world.

@jonashaag jonashaag mentioned this pull request Aug 28, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Flakey Test Flakey Test in CI - Unclosed File Handle
4 participants