Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10472 closed (fixed)

RegexURLResolver.reverse() is not threadsafe

Reported by: tdterry Owned by: tdterry
Component: Core (Other) Version: 1.0
Severity: Keywords: urlresolvers, reverse, threading
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I have found an incomplete-initialization bug in RegexURLResolver.reverse() when running under Apache mpm_worker with mod_wsgi.

RegexURLResolver objects are stored in process local memory using @memoize. The _reverse_dict is built on demand, so two threads trying to build it for the first time can run into an incomplete initialization. The attached patch changes RegexURLResolver._get_reverse_dict() to do an atomic set of self._reverse_dict, preventing multiple threads from seeing an inconsistent _reverse_dict.

Testing requires a multi-threaded setup and a good load test. These are not available in the framework, so I have not included a specific test with the patch.

Attachments (1)

thread-safe-urlresolvers.diff (1.7 KB) - added by tdterry 6 years ago.
Patch against r10030 to solve threadsafe issues with the urlresolvers.RegexURLResolver

Download all attachments as: .zip

Change History (8)

Changed 6 years ago by tdterry

Patch against r10030 to solve threadsafe issues with the urlresolvers.RegexURLResolver

comment:1 Changed 6 years ago by tdterry

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to tdterry
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 6 years ago by mrts

  • Component changed from Uncategorized to Core framework
  • Keywords threading added; threadsafe removed
  • Triage Stage changed from Unreviewed to Accepted

Pushing to accepted, the bug is real (strangely I haven't been hit by this running under the same setup) and the patch looks OK.

comment:3 Changed 6 years ago by mrts

  • Triage Stage changed from Accepted to Unreviewed

Looking around a bit more thoroughly, I didn't find no places where RegexURLResolver instances are shared between threads. Main use is via BaseHandler.get_response() where the BaseHandler instance creates the resolver instance anew for each request. The ones created within urls.py (conf/urls/defaults.py) are initialized before requests come in and not modified after (correct me in the latter).

Pushing back to unreviewed, IMHO can be closed as invalid.

comment:4 Changed 6 years ago by mrts

tdterry, I assumed you were actually hit by a bug, not doing code review. If you indeed were, please provide more context.

comment:5 Changed 6 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10037]) Fixed #10472 -- Fixed a race condition in reverse URL resolving.

This only shows up in for reverse() (not forwards resolving), since that
path uses a globally shared resolver object. Based on a patch from
Travis Terry.

comment:6 Changed 6 years ago by mtredinnick

(In [10039]) [1.0.X] Fixed #10472 -- Fixed a race condition in reverse URL resolving.

This only shows up in for reverse() (not forwards resolving), since that
path uses a globally shared resolver object. Based on a patch from
Travis Terry.

Backport of r10037 from trunk.

comment:7 Changed 6 years ago by mrts

tdterry, sorry, I missed the memoize on line 73, should have read the ticket description more carefully.

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