◐ Shell
reader mode source ↗
Skip to content

bpo-36918: Don't close the object which is again closed by destructor#13317

Closed
tirkarthi wants to merge 3 commits into
python:masterfrom
tirkarthi:bpo18748
Closed

bpo-36918: Don't close the object which is again closed by destructor#13317
tirkarthi wants to merge 3 commits into
python:masterfrom
tirkarthi:bpo18748

Conversation

@tirkarthi

@tirkarthi tirkarthi commented May 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

At the end of test BytesIO destructor is called that calls flush and close. Closing the object when the internal counter in FakeSocket gets to zero causes flush to be called on closed object in destructor. Mock the close function and let destructor do the work instead.

https://bugs.python.org/issue36918

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels May 14, 2019
@tirkarthi tirkarthi changed the title bpo-18478: Don't close the object which is again closed by destructor May 14, 2019
@tirkarthi tirkarthi changed the title bpo-18748: Don't close the object which is again closed by destructor May 14, 2019
@tirkarthi

tirkarthi commented May 26, 2019

Copy link
Copy Markdown
Member Author

@asvetlov I would wait since Serhiy had a different opinion on this at https://bugs.python.org/msg342709

@vstinner

vstinner commented Jun 11, 2019

Copy link
Copy Markdown
Member

I wrote a simpler change: PR #13955.

@vstinner

vstinner commented Jun 12, 2019

Copy link
Copy Markdown
Member

I wrote a simpler change: PR #13955.

This PR was wrong. I wrote PR #13996 which is very similar to this PR but is more explicit that only the mock is affected.

@vstinner

vstinner commented Jun 12, 2019

Copy link
Copy Markdown
Member

Thanks a lot @tirkarthi for proposing this PR and identifying the root cause of these new errors. I merged PR #13996 which is similar (but different :-)) than your PR.

@vstinner vstinner closed this Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@asvetlov asvetlov asvetlov approved these changes

@orsenthil orsenthil Awaiting requested review from orsenthil

@gpshead gpshead Awaiting requested review from gpshead

Assignees

No one assigned

Labels

awaiting merge skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tirkarthi @vstinner @asvetlov @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot