Skip to content

Fixes #126: generator objects leak on Python 3 #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Apr 2, 2014

The issue here is that generator objects that don't run to completion are uncollectable. There are a few places where iter_errors is called simply to retrieve the first error, and thus iter_errors is left uncompleted and holding references to a bunch of things (including self, which prevents deletion of the iter_errors generator instance).

The fix here isn't terribly elegant: It adds a kwarg to iter_errors to yield only the first error. Then, this is used from the places that only want the first error (along with list to fully exhaust the generator). The alternative (to just use list) would mean the possibly expensive creation of many error results only to have them immediately ignored and thrown away -- less efficient, but a simpler/less hackish solution perhaps.

Lastly, I couldn't figure out how to write a test for this: except perhaps to do:

warnings.filterwarnings("error", ".*", ResourceWarning)

which would raise an exception at the end of the test run rather than just a warning.

@Julian
Copy link
Member

Julian commented Apr 13, 2014

First of all, thanks a ton for taking a shot at this. Not the type of issue that contributors typically are willing to try and tackle, so :).

That being said, I'm skeptical that partially-run generators are uncollectable. References that are being held by the generator shouldn't prevent the generator from being collected -- but I haven't produced a minimal reproducing example one way or the other yet

Either way, I agree that the fix isn't really that pretty, if this were true, I'd be inclined to "fix" it by making the suite always exhaust its generators to tear down, not by changing actual library code (beside the fact that "private" function attributes are unfortunately a lie :( ).

I'm gonna take a closer look at this, but I also notice that the place where I was noticing this happen doesn't appear to be complaining anymore -- I don't see ResourceWarnings when running the test suite under 3.4. Not sure if anything has changed or not. How did you re-notice this?

@mdboom
Copy link
Contributor Author

mdboom commented Apr 17, 2014

I noticed this as a result of running the test suite in my own project: spacetelescope/pyfinf (I see it both with jsonschema 2.3.0 and git master). It turns out, however, that I only see this on Python 3.3.2, not Python 3.4.0. I think my Python 3 userbase tends to upgrade pretty quickly -- it's Python 2 where legacy support is more important -- so I'm fine with this being marked as "won't fix" maybe with some sort of note about Python 3 prior to 3.4.

I think the relevant change is this:

http://legacy.python.org/dev/peps/pep-0442/

given that jsonschema does have some generators with finally clauses in them, which were reportedly uncollectable when part of cycles prior to Python 3.4.

@Julian
Copy link
Member

Julian commented Apr 17, 2014

Ah got it, I actually have lost access to 3.3, but that was my suspicion, thanks for finding the relevant PEP.

If it in fact is something fixed in 3.4 I'd be inclined to just say yeah we should leave it as-is, but I'll do a bit more research here before closing.

Again, very much appreciated.

@Julian Julian closed this Jul 4, 2014
dlax added a commit to dlax/jsonschema that referenced this pull request Jun 8, 2017
73a0593 Merge pull request python-jsonschema#169 from epoberezkin/draft6-tests-ajv
e2e06d7 update ajv version, test
bd14545 Merge branch 'master' into draft6-tests-ajv
8758156 Merge pull request python-jsonschema#172 from epoberezkin/bignum
19a0b46 Merge pull request python-jsonschema#171 from epoberezkin/id
b8165b7 draft-06: option/bignum tests exclusiveMaximum/Minimum updated
f04ed0e Merge branch 'master' into draft6-tests-ajv
0931c60 test: run draft-04/06 tests with ajv
da8b14e Merge pull request python-jsonschema#170 from epoberezkin/boolean
20d706c Merge pull request python-jsonschema#163 from epoberezkin/boundary-point
ae72865 Merge pull request python-jsonschema#168 from korzio/patch-1
f809e51 draft-06: $id keyword
04d7e06 Add djv validator to readme
e86adb2 draft-06: $ref to boolean schemas
d62b754 draft-06: allOf, anyOf, oneOf keywords with boolean schemas
b714a18 draft-06: not keyword with boolean schemas
b6b00f9 draft-06: contains keyword with boolean schemas
9c05d18 draft-06: propertyNames keyword with boolean schemas
fa99135 draft-06: patternProperties keyword with boolean schemas
5a888a6 draft-06: dependencies keyword with boolean subschemas
53858ff draft-06: items keyword with boolean schemas
afd6fab Merge pull request python-jsonschema#167 from json-schema-org/revert-123-relative-ref-id
3b9b688 Revert "Test relative reference resolution when ID is not present"
77e0411 draft-06: properties keyword with boolean schemas
cff24c1 Merge pull request python-jsonschema#123 from yuloh/relative-ref-id
e7c1f4e Merge pull request python-jsonschema#161 from epoberezkin/items
43bfc6b Merge pull request python-jsonschema#165 from epoberezkin/test-schema
6956f20 draft-06: boolean root schema
e1139a3 update test-schema to draft-04
042fae9 Merge pull request python-jsonschema#164 from epoberezkin/zero-terminated
78de3a6 draft-06: zero-terminated float is a valid integer
9d998bb draft-04: added maximum/minimum tests for boundary point
b8f51ab Merge pull request python-jsonschema#151 from epoberezkin/exclusive-limits
951bd41 Merge pull request python-jsonschema#154 from epoberezkin/property-names
b974907 Merge pull request python-jsonschema#159 from epoberezkin/empty-property-list
9b1364e Merge pull request python-jsonschema#158 from epoberezkin/contains
95b4648 Merge pull request python-jsonschema#157 from epoberezkin/const
85efafb draft-06: required and dependencies with empty property arrays
7e40f2e draft-06: exclusiveMaximum and exclusiveMinimum validation
0058644 draft-06: propertyNames validation
ba6c582 draft-06: const validation
9867b96 draft-06: contains keyword validation
96fed74 draft-04/06: updated descriptions for items/additionalItems test cases
0799212 Make the URI tests consistent with themselves.
65c18d5 draft-04/06: items/additionalItems tests
12ab007 Update to the new organization.
a599e1e Flip protocol-relative URI Reference (not a URI)
ed3391c Merge pull request python-jsonschema#150 from handrews/draft6
555dfd3 Initialize draft6 tests from draft4.
5363098 Merge remote-tracking branch 'pboettch/master'
2ba2657 Merge remote-tracking branch 'agebhar1/feature/java+everit-org'
f7cec5a Merge remote-tracking branch 'agebhar1/feature/java+networknt'
16a8c1b Merge pull request python-jsonschema#145 from agebhar1/feature/java+json-schema-validator
6d29eb2 add `networknt/json-schema-validator` to test suite users
b38690a add `everit-org/json-schema` to test suite users
5637345 update Java's `json-schema-validator` repository
5a2c708 Added 'Modern C++ JSON schema validator'
371977c Merge branch 'develop'
e2618a0 Merge pull request python-jsonschema#142 from pipobscure/refOnly
b24fae2 When $ref is present other keywords should be ignored
cb25a4e Merge pull request python-jsonschema#134 from zloster/develop
1e9db5e Add optional check for non-ECMA 262 regular expressions
efb3c89 Merge pull request python-jsonschema#127 from iainbeeston/extra-items-tests
27692ac Merge pull request python-jsonschema#133 from gavinwahl/develop
fc7584e add postgres-json-schema
654cc26 Added tests for items when data contains more or less types than the schema
5fb3d9f Merge pull request python-jsonschema#125 from yuloh/ref-property
833a70c Merge pull request python-jsonschema#126 from gregsdennis/develop
302da1e Added Manatee.Json as .Net implementor.
dc001c2 Add test for properties named $ref
6d366dc Test relative reference resolution
9355965 Merge pull request python-jsonschema#120 from seagreen/extract-test-schema
42ba0f9 Extract the schema for tests into a JSON file.
2f0d6e3 Merge pull request python-jsonschema#118 from yuloh/ignores-non-objects-required
cc52997 Test required ignores non objects
27ed651 Merge pull request python-jsonschema#117 from tatut/develop
c9f5dcd Add Clojure json-schema to users
8549899 Merge pull request python-jsonschema#116 from yuloh/patch-1
de70a75 Add league/json-guard implementation for PHP
9723c8f Merge pull request python-jsonschema#111 from Relequestual/patch-1
9d5909a Also check string "1" is not a number in draft 4
e4ab5bd Check that a string of "1" is not a number
5dc71b8 Merge pull request python-jsonschema#108 from duksis/patch-1
6a054cd Adds ex_json_schema validator for Elixir
f3d5aeb Merge branch 'develop'
5bcf11a Port to draft4.
3f7ed81 Merge pull request python-jsonschema#103 from Relequestual/patch-1
928d3b0 Fixed incorrect negative description of a sub test
62414e4 Minor shuffling.
a7305a6 Merge remote-tracking branch 'legoktm/tox' into develop
5cc622c Merge pull request python-jsonschema#100 from atomiqio/develop
2f51b2e updated node.js info in README.md
8f29757 Add the README note on develop.
338eef3 Merge pull request python-jsonschema#97 from atomiqio/develop
146e291 JavaScript should written in upper camel case
7511038 Merge pull request python-jsonschema#95 from epoberezkin/develop
ca28bbb added ajv validator to readme
504f776 Merge pull request python-jsonschema#93 from gelraen/readme
59207fd Add another Go implementation
d14cf96 Use tox to run tests

git-subtree-dir: json
git-subtree-split: 73a05935f5418f4b59fb8084b4ffa6edf8a4eea0
Julian added a commit that referenced this pull request Jun 11, 2017
27f8c84 Merge pull request #184 from remexre/master
8fc497e Changes draft06 tests to use draft06 metaschema.
583ecf9 Add name.json to the CLI tool's remotes.
67c7b4d Merge pull request #180 from bismark/patch-1
784198b Update erlang URL
05fdba4 Merge pull request #173 from epoberezkin/format
e8ef6fd draft-06: additional format tests
d545553 Merge branch 'master' into format
bc4de6c Merge pull request #174 from epoberezkin/zero-term
f455ecc Merge branch 'master' into zero-term
5f6abf7 draft-06: zero-terminated float test description
c1b12bf Merge pull request #160 from epoberezkin/ref-tests
25836f7 update draft-06 tests: "id" -> "$id"
671bad6 fix: JavaScript test
44f6447 Merge branch 'master' into ref-tests
73a0593 Merge pull request #169 from epoberezkin/draft6-tests-ajv
e2e06d7 update ajv version, test
bd14545 Merge branch 'master' into draft6-tests-ajv
cbe0e5b Merge branch 'master' into ref-tests
8758156 Merge pull request #172 from epoberezkin/bignum
19a0b46 Merge pull request #171 from epoberezkin/id
e1e1eec draft-06: format "json-pointer" tests
1e2834c draft-06: format "uri-template" tests
21b776e draft-06: format "uri-reference" tests
b8165b7 draft-06: option/bignum tests exclusiveMaximum/Minimum updated
f04ed0e Merge branch 'master' into draft6-tests-ajv
0931c60 test: run draft-04/06 tests with ajv
da8b14e Merge pull request #170 from epoberezkin/boolean
20d706c Merge pull request #163 from epoberezkin/boundary-point
ae72865 Merge pull request #168 from korzio/patch-1
f809e51 draft-06: $id keyword
04d7e06 Add djv validator to readme
e86adb2 draft-06: $ref to boolean schemas
d62b754 draft-06: allOf, anyOf, oneOf keywords with boolean schemas
b714a18 draft-06: not keyword with boolean schemas
b6b00f9 draft-06: contains keyword with boolean schemas
9c05d18 draft-06: propertyNames keyword with boolean schemas
fa99135 draft-06: patternProperties keyword with boolean schemas
5a888a6 draft-06: dependencies keyword with boolean subschemas
53858ff draft-06: items keyword with boolean schemas
afd6fab Merge pull request #167 from json-schema-org/revert-123-relative-ref-id
3b9b688 Revert "Test relative reference resolution when ID is not present"
77e0411 draft-06: properties keyword with boolean schemas
cff24c1 Merge pull request #123 from yuloh/relative-ref-id
e7c1f4e Merge pull request #161 from epoberezkin/items
43bfc6b Merge pull request #165 from epoberezkin/test-schema
6956f20 draft-06: boolean root schema
e1139a3 update test-schema to draft-04
042fae9 Merge pull request #164 from epoberezkin/zero-terminated
78de3a6 draft-06: zero-terminated float is a valid integer
9d998bb draft-04: added maximum/minimum tests for boundary point
b8f51ab Merge pull request #151 from epoberezkin/exclusive-limits
951bd41 Merge pull request #154 from epoberezkin/property-names
b974907 Merge pull request #159 from epoberezkin/empty-property-list
9b1364e Merge pull request #158 from epoberezkin/contains
95b4648 Merge pull request #157 from epoberezkin/const
85efafb draft-06: required and dependencies with empty property arrays
7e40f2e draft-06: exclusiveMaximum and exclusiveMinimum validation
0058644 draft-06: propertyNames validation
ba6c582 draft-06: const validation
9867b96 draft-06: contains keyword validation
96fed74 draft-04/06: updated descriptions for items/additionalItems test cases
0799212 Make the URI tests consistent with themselves.
65c18d5 draft-04/06: items/additionalItems tests
3186761 draft-04/06: root ref "#" in remote ref
0178c74 draft-04/06: recursive refs test
a884acc draft-04/06: base URI change tests
12ab007 Update to the new organization.
a599e1e Flip protocol-relative URI Reference (not a URI)
ed3391c Merge pull request #150 from handrews/draft6
555dfd3 Initialize draft6 tests from draft4.
5363098 Merge remote-tracking branch 'pboettch/master'
2ba2657 Merge remote-tracking branch 'agebhar1/feature/java+everit-org'
f7cec5a Merge remote-tracking branch 'agebhar1/feature/java+networknt'
16a8c1b Merge pull request #145 from agebhar1/feature/java+json-schema-validator
6d29eb2 add `networknt/json-schema-validator` to test suite users
b38690a add `everit-org/json-schema` to test suite users
5637345 update Java's `json-schema-validator` repository
5a2c708 Added 'Modern C++ JSON schema validator'
371977c Merge branch 'develop'
e2618a0 Merge pull request #142 from pipobscure/refOnly
b24fae2 When $ref is present other keywords should be ignored
cb25a4e Merge pull request #134 from zloster/develop
1e9db5e Add optional check for non-ECMA 262 regular expressions
efb3c89 Merge pull request #127 from iainbeeston/extra-items-tests
27692ac Merge pull request #133 from gavinwahl/develop
fc7584e add postgres-json-schema
654cc26 Added tests for items when data contains more or less types than the schema
5fb3d9f Merge pull request #125 from yuloh/ref-property
833a70c Merge pull request #126 from gregsdennis/develop
302da1e Added Manatee.Json as .Net implementor.
dc001c2 Add test for properties named $ref
6d366dc Test relative reference resolution
9355965 Merge pull request #120 from seagreen/extract-test-schema
42ba0f9 Extract the schema for tests into a JSON file.
2f0d6e3 Merge pull request #118 from yuloh/ignores-non-objects-required
cc52997 Test required ignores non objects
27ed651 Merge pull request #117 from tatut/develop
c9f5dcd Add Clojure json-schema to users
8549899 Merge pull request #116 from yuloh/patch-1
de70a75 Add league/json-guard implementation for PHP
9723c8f Merge pull request #111 from Relequestual/patch-1
9d5909a Also check string "1" is not a number in draft 4
e4ab5bd Check that a string of "1" is not a number
5dc71b8 Merge pull request #108 from duksis/patch-1
6a054cd Adds ex_json_schema validator for Elixir
f3d5aeb Merge branch 'develop'
5bcf11a Port to draft4.
3f7ed81 Merge pull request #103 from Relequestual/patch-1
928d3b0 Fixed incorrect negative description of a sub test
62414e4 Minor shuffling.
a7305a6 Merge remote-tracking branch 'legoktm/tox' into develop
5cc622c Merge pull request #100 from atomiqio/develop
2f51b2e updated node.js info in README.md
8f29757 Add the README note on develop.
338eef3 Merge pull request #97 from atomiqio/develop
146e291 JavaScript should written in upper camel case
7511038 Merge pull request #95 from epoberezkin/develop
ca28bbb added ajv validator to readme
504f776 Merge pull request #93 from gelraen/readme
59207fd Add another Go implementation
d14cf96 Use tox to run tests

git-subtree-dir: json
git-subtree-split: 27f8c840acdb3700de7173981e95ed90ef28de2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants