bpo-36918: Don't close the object which is again closed by destructor by tirkarthi · Pull Request #13317 · python/cpython

bpo-36918: Don't close the object which is again closed by destructor by tirkarthi · Pull Request #13317 · python/cpython

Conversation

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

tirkarthi changed the title ~~bpo-18478: Don't close the object which is again closed by destructor~~ bpo-18748: Don't close the object which is again closed by destructor

tirkarthi changed the title ~~bpo-18748: Don't close the object which is again closed by destructor~~ bpo-36918: Don't close the object which is again closed by destructor

		''')
		try:
		# Silence destructor error
		http.client.HTTPConnection.close = lambda self: None

Choose a reason for hiding this comment

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

This will affect all instances of HTTPConnection.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Why not adding a close() method to FakeHTTPConnection?

Choose a reason for hiding this comment

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

There are no test case failures on changing close in FakeHTTPConnection but I haven't had time to analyze it well to see if there are any other cases it affects. So I kept the change minimal to have reported tests silenced.

Choose a reason for hiding this comment

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

After I wrote a similar change, I now understand this change and I agree that it's correct. But the change is surprising :-) I wrote PR #13996 which adds a "mock_close" attribute to fake_http(): if mock_close is true, a close() method which does nothing is defined. IMHO it's less surprising, since it's more explicitly that only the mock is affected.

I wrote a simpler change: PR #13955.

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.

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.