Opened 8 years ago

Closed 8 years ago

Last modified 8 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 8 years ago.
Patch against r10030 to solve threadsafe issues with the urlresolvers.RegexURLResolver

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by tdterry

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

comment:1 Changed 8 years ago by tdterry

Owner: changed from nobody to tdterry
Status: newassigned

comment:2 Changed 8 years ago by mrts

Component: UncategorizedCore framework
Keywords: threading added; threadsafe removed
Triage Stage: UnreviewedAccepted

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 8 years ago by mrts

Triage Stage: AcceptedUnreviewed

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 8 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 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(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 8 years ago by Malcolm Tredinnick

(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 8 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