Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#14433 closed (fixed)

Thread safety problem in contrib.sitemaps

Reported by: gabrielhurley Owned by: gabrielhurley
Component: Contrib apps Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Since sitemaps are global objects, setting self.request on the object is thread-unsafe. Reported here by Justin Bronn.

Attachments (1)

14433.diff (4.8 KB) - added by gabrielhurley 4 years ago.
Restores thread-safety to contrib.sitemaps

Download all attachments as: .zip

Change History (7)

Changed 4 years ago by gabrielhurley

Restores thread-safety to contrib.sitemaps

comment:1 Changed 4 years ago by gabrielhurley

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

I believe this patch should restore thread safety by passing site as an argument rather than storing request on the Sitemap object. It is as backwards-compatible as possible, since prior to [13980] the get_urls method simply called Site.objects.get_current() directly, so this patch uses that behavior as a fallback for anyone who may have written custom view code which does not pass in a site as an argument.

comment:2 Changed 4 years ago by lukeplant

Thanks Gabriel. I will commit with changes:

  • added tests for raising ImproperlyConfigured (and then added an import and some other logic that made the tests pass - hurray for TDD :-)
  • used keyword arguments in to Sitemap.get_urls() instead of positional, in django/contrib/sitemaps/views.py and django/contrib/gis/sitemaps/views.py

I am unable to run the GIS testsuite (see #14439), so I have not checked that the GIS sections are correct, but they look correct are (hopefully) trivial.

comment:3 Changed 4 years ago by lukeplant

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

(In [14141]) Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

comment:4 Changed 4 years ago by lukeplant

(In [14142]) [1.2.X] Fixed #14433 - replaced a thread-unsafe solution to #10235 introduced in [13980]

This patch also addresses sitemap code found in contrib/gis, which [13980]
did not.

Thanks to gabrielhurley for the initial patch.

Refs #10235, #14386

Backport of [14141] from trunk.

comment:5 Changed 4 years ago by gabrielhurley

I knew the patch wasn't finished, but I had to run out the door. I wanted to give Justin or someone else who could run the GIS suite a chance to look at it. All the same, thanks for wrapping it up Luke.

comment:6 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.