bpo-30980: Fix double close in asyncore.file_wrapper#2789
Merged
Conversation
test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective.
|
@nirs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @giampaolo, @ezio-melotti and @pitrou to be potential reviewers. |
Contributor
Author
|
@Haypo, please review. |
Invalidated self.fd before closing, handling correctly the case when os.close raises.
Member
|
I don't understand this change, can you please elaborate? It make the test failing. Is it supposed to fix a bug? |
nirs
changed the title
Contributor
Author
|
@Haypo the first patch added a failing test to demonstrate the issue, the second patch fixes it. |
Member
|
The test now leaks a file descriptor: |
vstinner
reviewed
Jul 20, 2017
| @@ -431,9 +431,11 @@ def test_resource_warning(self): | |||
| def test_close_twice(self): | |||
| fd = os.open(support.TESTFN, os.O_RDONLY) | |||
Member
There was a problem hiding this comment.
Add something like: self.addCleanup(os.close, fd). It's enough to fix "./python -m test -R 3:3 test_asyncore".
vstinner
approved these changes
Jul 21, 2017
vstinner
reviewed
Jul 24, 2017
| f.close() | ||
| os.close(f.fd) # file_wrapper dupped fd | ||
| with self.assertRaises(OSError): | ||
| f.close() |
Member
There was a problem hiding this comment.
Please add this test here:
self.assertEqual(f.fileno(), -1)
To test your fix.
nirs
commented
Jul 24, 2017
| with self.assertRaises(OSError): | ||
| f.close() | ||
|
|
||
| self.assertEqual(f.fd, -1) |
Contributor
Author
There was a problem hiding this comment.
@Haypo isn't this ^^^^ good enough?
Since file_wrapper.fd is not documented, maybe we should replace f.fd with f.fileno()? Do you want to add this change to the patch?
Member
There was a problem hiding this comment.
Oh, I missed it :-) It's enough.
Member
|
Do you want to backport your fix to 3.5 and 3.6? |
Member
|
(ah, and 2.7 too according your bpo.) |
Contributor
Author
|
@Haypo , I'll send backports later this week, thanks! |
nirs
added a commit
to nirs/cpython
that referenced
this pull request
Jul 26, 2017
* bpo-30980: Fix close test to fail test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective. * bpo-30980: Fix double close protection Invalidated self.fd before closing, handling correctly the case when os.close raises. * bpo-30980: Fix fd leak introduced in the fixed test
nirs
added a commit
to nirs/cpython
that referenced
this pull request
Jul 26, 2017
* bpo-30980: Fix close test to fail test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective. * bpo-30980: Fix double close protection Invalidated self.fd before closing, handling correctly the case when os.close raises. * bpo-30980: Fix fd leak introduced in the fixed test
nirs
added a commit
to nirs/cpython
that referenced
this pull request
Jul 26, 2017
* bpo-30980: Fix close test to fail test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective. * bpo-30980: Fix double close protection Invalidated self.fd before closing, handling correctly the case when os.close raises. * bpo-30980: Fix fd leak introduced in the fixed test
vstinner
pushed a commit
that referenced
this pull request
Jul 26, 2017
* bpo-30980: Fix close test to fail test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective. * bpo-30980: Fix double close protection Invalidated self.fd before closing, handling correctly the case when os.close raises. * bpo-30980: Fix fd leak introduced in the fixed test
vstinner
pushed a commit
that referenced
this pull request
Jul 26, 2017
* bpo-30980: Fix close test to fail test_close_twice was not considering the fact that file_wrapper is duping the file descriptor. Closing the original descriptor left the duped one open, hiding the fact that close protection is not effective. * bpo-30980: Fix double close protection Invalidated self.fd before closing, handling correctly the case when os.close raises. * bpo-30980: Fix fd leak introduced in the fixed test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_close_twice was not considering the fact that file_wrapper is
duping the file descriptor. Closing the original descriptor left the
duped one open, hiding the fact that close protection is not effective.