Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlock in LRUCache and improve test coverage #1000

Merged

Conversation

@EtiennePelletier
Copy link
Contributor

EtiennePelletier commented May 9, 2019

I was adding tests for Undefined and other utils by running coverage and finding which lines did not have test coverage, then I found there is a deadlock bug in the LRUCache's setdefault method.

As documented in the commit: setdefault was acquiring write_lock, then calling getitem and also
potentially setitem, which both try to acquire the write lock.
Minimal snippet to reproduce the bug with Python 3.7.3 and Jinja 2.10.1:

from jinja2.utils import LRUCache
x = LRUCache(1)
x.setdefault(1)  # <-- deadlock :(

It's fixed now and we have better test coverage 馃槃

@EtiennePelletier EtiennePelletier force-pushed the EtiennePelletier:fix_deadlock_improve_tests branch 2 times, most recently from 0bdfed9 to 1dae748 May 9, 2019
tests/test_utils.py Outdated Show resolved Hide resolved
@kevin-brown kevin-brown dismissed their stale review Jun 25, 2019

Pull request has since been updated

@davidism davidism added this to the 2.11.0 milestone Oct 4, 2019
@davidism davidism force-pushed the EtiennePelletier:fix_deadlock_improve_tests branch from 73866b2 to 70fefa5 Oct 4, 2019
setdefault was acquiring write_lock, then calling getitem and also
potentially setitem, which also both try to acquire the write lock.
@davidism davidism force-pushed the EtiennePelletier:fix_deadlock_improve_tests branch from 70fefa5 to 69d8d98 Oct 4, 2019
@davidism davidism merged commit d3b976b into pallets:master Oct 4, 2019
10 checks passed
10 checks passed
Tests Build #20191004.3 succeeded
Details
Tests (Job Docs) Job Docs succeeded
Details
Tests (Job PyPy 3 Linux) Job PyPy 3 Linux succeeded
Details
Tests (Job Python 2.7 Linux) Job Python 2.7 Linux succeeded
Details
Tests (Job Python 2.7 Windows) Job Python 2.7 Windows succeeded
Details
Tests (Job Python 3.5 Linux) Job Python 3.5 Linux succeeded
Details
Tests (Job Python 3.6 Linux) Job Python 3.6 Linux succeeded
Details
Tests (Job Python 3.7 Linux) Job Python 3.7 Linux succeeded
Details
Tests (Job Python 3.7 Mac) Job Python 3.7 Mac succeeded
Details
Tests (Job Python 3.7 Windows) Job Python 3.7 Windows succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.