Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#19905 closed Bug (fixed)

ResourceWarning in formtools tests

Reported by: Florian Apolloner Owned by: nobody
Component: contrib.formtools Version: dev
Severity: Release blocker Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

running formtools tests with

PYTHONPATH=.. python3.3 -Wall ./runtests.py --settings=test_sqlite --verbosity=2 formtools

yields

/usr/lib/python3.3/unittest/case.py:385: ResourceWarning: unclosed file <_io.BufferedReader name='/tmp/django_512q8m/tmpjkx__v/tests_6.py'>
  function()

quite often. I couldn't yet determine if that's a bug in the wizard code or our tests.

Attachments (1)

19905-1.diff (5.2 KB ) - added by Claude Paroz 10 years ago.
Fixing test warnings

Download all attachments as: .zip

Change History (17)

comment:1 by Aymeric Augustin, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Baptiste Mispelon, 11 years ago

I took a peek at the tests and I'm left a bit puzzled.

The name of the unclosed files is consistent with what happens in the test (lots of open(__file__)).

However, I don't really understand the flow of the code: it seems some files are closed before being re-opened (https://github.com/django/django/blob/master/django/contrib/formtools/tests/wizard/namedwizardtests/tests.py#L125-L126).

I found commit 64a3c7f9aeda347a72e3f1e8e88381b5b3d479d8 which seems like an attempt to fix those issues (though the commit message is not very optimistic).

I also found a line in formtools.wizard.storage.base.BaseStorage.get_step_files() which is a bit suspicious: https://github.com/django/django/blob/master/django/contrib/formtools/wizard/storage/base.py#L81 but I'm not familiar enough with formtools' code to tell whether that opened file gets closed later on.

comment:3 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added

comment:4 by Baptiste Mispelon, 11 years ago

I've made a bit of progress on my branch: https://github.com/bmispelon/django/compare/ticket-19905

Moving the opening/closing of files to setUp/tearDown, I managed to reduce the number of ResourceWarning from 26 to 20.

The command I use for counting the warnings is the following: PYTHONPATH=.. python3 -Wall ./runtests.py --settings=test_sqlite --verbosity=2 formtools 2>&1 | grep ResourceWarning | wc -l (from django/tests).

edit: No I didn't, i just made two tests fail because of a NameError, which I didn't catch because i was just grepping the results.

Oh well, back to the drawing board. Sorry for the noise.

I did learn that NamedWizardTests causes 5 ResourceWarning every time it's run (2 for test_form_finish and 3 for test_cleaned_data) so there's that.

Last edited 11 years ago by Baptiste Mispelon (previous) (diff)

comment:5 by Baptiste Mispelon, 11 years ago

I made a test using io.BytesIO instead of opening actual files and the number or warnings doesn't change.

This seems like a pretty conclusive evidence that the problem lies with formtools' code itself rather than the tests.

Here's the list of the methods that leak file descriptors:

  • NamedWizardTests.test_form_finish: 2
  • NamedWizardTests.test_cleaned_data: 3
  • WizardTests.test_form_finish: 2
  • WizardTests.test_cleaned_data: 3
  • WizardTests.test_form_reresh: 3

As each class is ran twice (once for session and once for cookie-based wizard), that amounts to 26 leaked descriptors.

comment:6 by Kevin Christopher Henry, 11 years ago

For what it's worth, I wasn't able to duplicate these ResourceWarnings (in Python 2.7.5 and 3.3.2 under Windows).

Taking a quick glance at the code, the only thing that jumps out at me is that formtools.wizard.storage.base.BaseStorage.get_step_files() opens temp files that get passed along (via current_step_files). I don't know enough about this code to say when or whether these file handles get closed, but I thought I'd mention it.

comment:7 by Claude Paroz, 10 years ago

Basically, the explanation of this behavior is rather straightforward: at various steps, formtools is reopening files (in get_step_files) to provide them to the wizard form(s), a bit like a standard UploadHandler does it for data coming from a request to populate request.FILES (and then form files data).
If we look at the same process with standard forms, we can see that we don't explicitly close the files either (not an issue with InMemoryUploadedFile, not sure about TemporaryUploadedFile for bigger files). In standard form tests, it's easy to manually close files as we are opening them ourselves. It's different in formtools tests, as those files are reopened inside formtools code.

This leaves us with two paths:

  1. We don't think that not explicitly closing files in this situation is a problem, and we should then simply silence the warnings (didn't try if it's possible or not) or live with them.
  2. We do think that open files provided to forms with file fields should be explicitly closed at some point in the code (and not artifically in test code). But then the resolution is not in formtools, but in the form infrastructure itself.

I have doubts that 2 is feasible, so I'm afraid we are left with 1.

by Claude Paroz, 10 years ago

Attachment: 19905-1.diff added

Fixing test warnings

comment:8 by Claude Paroz, 10 years ago

Has patch: set

The attached patch is suppressing the warnings, but it's a test-only solution, that is the warnings will probably still be emitted in real user code.

comment:9 by Florian Apolloner, 10 years ago

comment:10 by Florian Apolloner <florian@…>, 10 years ago

Resolution: fixed
Status: newclosed

In c4c2c99669ef9d58306856c47058b121bbecfe5f:

Fixed #19905 -- Fixed leakage of file descriptors in form wizard.

comment:11 by Florian Apolloner <florian@…>, 10 years ago

In e3792bb95f3c3854e39269aeb1ff4c7b3e859e70:

[1.7.x] Fixed #19905 -- Fixed leakage of file descriptors in form wizard.

Backport of c4c2c99669ef9d58306856c47058b121bbecfe5f from master.

comment:12 by Claude Paroz, 10 years ago

Resolution: fixed
Status: closednew

Reopening, as the fix is causing failures. On Python 2 at least, __file__ can have the .pyc extension, so UPLOADED_FILE_NAME is not always equal to __file__. I guess the CI server is cleaning .pyc files before each run.

comment:13 by Tim Graham, 10 years ago

Has patch: unset
Severity: NormalRelease blocker

Yes, CI does git clean -fqdx before each run.

comment:14 by Andrew Godwin, 10 years ago

timo: Why did you mark this as a release blocker? We've clearly done releases containing it before...

comment:15 by Claude Paroz <claude@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 1aaf4053f5bda3e064448ca5abfa20eb3d029565:

Fixed formtools tests with Python 2

Fixes #19905 again.

comment:16 by Claude Paroz <claude@…>, 10 years ago

In d4623d13b731efef499b49576f883a5578297fdd:

[1.7.x] Fixed formtools tests with Python 2

Fixes #19905 again.
Backport of 1aaf4053f5 from master.

Note: See TracTickets for help on using tickets.
Back to Top