Opened 3 years ago

Closed 3 years ago

#8573 defect closed fixed (fixed)

Files should be explicitly closed

Reported by: mark williams Owned by: Ville Skyttä
Priority: normal Milestone: PyPy-support
Component: core Keywords:
Cc: Branch: 8573-scop-files
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

Across Twisted's code base you'll see this pattern:

data = open('somefile').read()

CPython's reference counting memory manager closes the file object created by open('somefile') almost immediately. But other implementations (namely PyPy) don't use reference counting, so this leaks a file descriptor for an indeterminate amount of time.

This pattern is especially bad in the case of writing a file:

open('somefile', 'w').write('data')

...because the code that immediately follows often expects somefile to exist and contain data. But there's no guarantee on PyPy when both will be true. This responsible for at least some of conch's test failures under PyPy.

The solution in this modern world is to replace all instances of this pattern with a context manager instead:

with open('somefile') as f:
    data = f.read()

with open('somefile', 'w') as f:
    f.write('data')

Change History (19)

comment:1 Changed 3 years ago by mark williams

Here's one PR that addresses this issue: https://github.com/twisted/twisted/pull/299

comment:2 Changed 3 years ago by Craig Rodrigues

I opened a separate 8574. If you want to close out any more open() related problems in this ticket, feel free. :)

comment:3 Changed 3 years ago by Ville Skyttä

Keywords: review added

comment:4 in reply to:  2 Changed 3 years ago by Ville Skyttä

Replying to rodrigc:

I opened a separate 8574. If you want to close out any more open() related problems in this ticket, feel free. :)

My attempts at replying to this are failing here, so I replied in GitHub instead: https://github.com/twisted/twisted/pull/306#issuecomment-231531055

comment:5 Changed 3 years ago by Adi Roiban

I assume that latest PR for this ticket is https://github.com/twisted/twisted/pull/308

comment:6 Changed 3 years ago by Adi Roiban

Keywords: pypy files with review removed
Owner: set to Ville Skyttä

Thanks for the changes. They look good.

We need coverage for all new changes, including changes to the existing code... all changes are equal :)

If an existing code is changed and it has no test in trunk, you should add a new test as part of the new branch.

If you really care about that code, you should do an extra effort and make sure we don't break that functionality in the future.

On top of that, if you really care about the functionality provided by that code on PYPY you should write a test, as otherwise there is no guarantee that the code will work on PYPY. The open() change might be straight forward but that code might have some other obscure PYPY specific bug and unless we don't have a test there is no way to tell if the functionality works or not in PYPY.

Adding an automated test will also increase the change of having someone else fixing that code for you.

I am not a PYPY user and if my change will break PYPY, unless there is a test for it, I will never know.

In the same time, if someone is reporting a bug in PYPY, if we have a pypy builder and a few tests for that code, it would be much easier for another developer to just update the existing tests (or add similar tests) to reproduce the error and try the fix it.

Twisted users are asking from the core Twisted developer to not break the existing functionalities while introducing new functionalities. I think that it is fair for the core twisted developers to ask for all changes to have tests to help prevent regressions.


I would argue that if a PYPY patch is not adding coverage, the code should be left as is and maybe create a new ticket to talk about PYPY issues and leave a comment linking to the new ticket.


If you still think that the code should be merge in trunk as it is now, feel free to put this ticket back for review so that another developer can review it.

Thanks!

comment:7 Changed 3 years ago by mark williams

Keywords: review added

comment:8 Changed 3 years ago by Adi Roiban

Hi. What is the branch / PR associated with this review?

comment:9 in reply to:  8 Changed 3 years ago by mark williams

Replying to adiroiban:

Hi. What is the branch / PR associated with this review?

Whoops, same as before: ​https://github.com/twisted/twisted/pull/306

WOOPS, -- ​https://github.com/twisted/twisted/pull/308

Specifically, I'm about to add my review per your recommendation:

If you still think that the code should be merge in trunk as it is now, feel free to put this ticket back for review so that another developer can review it.

Last edited 3 years ago by mark williams (previous) (diff)

comment:10 Changed 3 years ago by Adi Roiban

Hm... PR 306 is closed... AFAIK at least some code was merged with #8589

https://github.com/twisted/twisted/commit/2dd6d509987b0ea3936e86e77225836b1eaaeaf4

comment:11 Changed 3 years ago by mark williams

Keywords: review removed

Replying to adiroiban:

Thanks for the changes. They look good.

We need coverage for all new changes, including changes to the existing code... all changes are equal :) ...[snip]... On top of that, if you really care about the functionality provided by that code on PYPY you should write a test, as otherwise there is no guarantee that the code will work on PYPY. The open() change might be straight forward but that code might have some other obscure PYPY specific bug and unless we don't have a test there is no way to tell if the functionality works or not in PYPY.

While I'm all for increasing test coverage in general, and I'm certainly in favor of improving the testing situation for PyPy, we simply cannot measure code coverage under PyPy until a PyPy release comes out that includes this fix:

https://bitbucket.org/pypy/pypy/issues/2335

Adding an automated test will also increase the change of having someone else fixing that code for you.

I am not a PYPY user and if my change will break PYPY, unless there is a test for it, I will never know.

In the same time, if someone is reporting a bug in PYPY, if we have a pypy builder and a few tests for that code, it would be much easier for another developer to just update the existing tests (or add similar tests) to reproduce the error and try the fix it.

I intend to set up at least one functioning PyPy builder, but I can't say exactly when it will be ready.

Twisted users are asking from the core Twisted developer to not break the existing functionalities while introducing new functionalities. I think that it is fair for the core twisted developers to ask for all changes to have tests to help prevent regressions.

Yes, and I think the fact that Twisted takes that seriously is a huge reason why it's as good as it is.

But! Twisted's in an awkward position right now. We seem to be having issues with our build bots, code coverage reporting from Travis is unreliable, the code review process in the new world of Github pull requests is confusing, etc. In short, new, practical barriers make it difficult at best and impossible at worst to meet some of these requirements.

At the same time, Glyph has made it clear that Twisted can't freeze its development process while all this gets sorted out. So, we have two choices:

1) leave PRs like https://github.com/twisted/twisted/pull/308 open until the tooling gets fixed; 2) Use what indicators we have -- from very weak ones like the apparent triviality of the patch to stronger ones like the fact that related patches reduce the number of test failures -- to guide merges.

1) seems to be freezing development. 2) risks putting bad code into trunk, or at least code that would not pass quality requirements. But the batch of PRs that Craig has written and/or merged have improved Twisted's Python 3 support and general quality, without doing anything tricky. They are generally pretty easy to review -- like this one -- and they provide immediate value. I don't want to lose track of simple and effective changes because our tooling isn't ready. The tools are supposed to help, not hinder.


I would argue that if a PYPY patch is not adding coverage, the code should be left as is and maybe create a new ticket to talk about PYPY issues and leave a comment linking to the new ticket.


In this specific case, https://github.com/twisted/twisted/pull/308 does not improve coverage, nor does it include tests that ensure it works correctly on PyPy. But the Twisted project has no way to reliably verify this yet! And it consists of a near-mechanical translation from an incorrect file management mechanism to a correct one. And we know that this approach reduces the number of failures on PyPy. So to my mind, it is a perfect case for 2) above.

I'm perfectly OK with opening a new ticket to track PyPy coverage issues, but I don't think it's actually necessary. Once we can *actually* measure coverage reliably, I personally intend to start opening tickets that just improve coverage for all platforms I can get my hands on. In fact, that should be its own milestone if it isn't already.

If you still think that the code should be merge in trunk as it is now, feel free to put this ticket back for review so that another developer can review it.

I say merge it! I won't argue for this consistently or generally, but I will argue for it during this particular transition in Twisted's development practices, for patches that are particularly easy to review.

Thanks!

Thank you, Adi! This is a pretty confusing and difficult time to be a Twisted contributor, and I really appreciate the work you've done and continue doing, such as fighting with PyPy on Travis. I think we can get to a point pretty soon where our tools are at the level they need to be.

comment:12 Changed 3 years ago by Adi Roiban

Thanks for the arguments!

I have resolved the conflicts from PR 308 and pushed the merge with trunk.

Waiting for the tests and will merge. Thanks!

Let me know when a new PYPY release is available via pyenv and I will update the travis-ci job.

Travis-CI is fine... is just codecov.io which is a bit broken.

Thanks

comment:13 in reply to:  12 Changed 3 years ago by mark williams

Replying to adiroiban:

Thanks for the arguments!

Thank you for listening :)

I have resolved the conflicts from PR 308 and pushed the merge with trunk.

Waiting for the tests and will merge. Thanks!

Let me know when a new PYPY release is available via pyenv and I will update the travis-ci job.

Travis-CI is fine... is just codecov.io which is a bit broken.

Well, that sucks. Any ticket/mailing list discussion about using something else, like coveralls?

Thanks

comment:14 Changed 3 years ago by Adi Roiban

Branch: 8573-scop-files

comment:15 Changed 3 years ago by Craig Rodrigues

I tried to merge https://github.com/twisted/twisted/pull/308 after review approval from markrwilliams and was blocked by codecov.

comment:16 Changed 3 years ago by Adi Roiban

codecov/project is still required ... I think that you tried to merge it before all buildbot builders were done and in this case codecov/project is not ready yet.

As stated in comment:12 I was working on merging this.

Please let me know if you want to merge it.

comment:17 Changed 3 years ago by Craig Rodrigues

If you are going to merge it, then merge it.

I tried to merge it but was blocked by codecov. I have no idea what you are doing with codecov.

comment:18 Changed 3 years ago by Adi Roiban

Well.. I was waiting for the tests to be done... but then I saw the rebase.

codecov/project is not disabled. codecov should no longer block a merge.

Please try again to merge it. Thanks!

comment:19 Changed 3 years ago by GitHub <noreply@…>

Resolution: fixed
Status: newclosed

In 778eda4f:

Merge pull request #308 from twisted/8573-scop-files

Author: scop
Reviewer: rodrigc
Fixes: #8573

More file close related fixes

Note: See TracTickets for help on using tickets.