Possible Bug in RegexURLResolver

2016-07-11 Thread a . branco
Hello everyone. As suggested by Markus on django-users group, I'm posting this here too. --- I'm using django (1, 10, 0, u'beta', 1). When I try to reverse url in shell everything goes fine. When under nginx/uwsgi with many concurrent request I get ... /local/lib/python2.7/site-packages/

Re: Possible Bug in RegexURLResolver

2016-07-11 Thread Markus Holtermann
Hey, thanks for posting this here. I opened https://code.djangoproject.com/ticket/26888 to keep track of this. Can I get somebody with threading experience in Python tell me if your proposed patch makes sense? Also, I marked this as a release blocker for 1.10 as I introduced this during a patch

Re: Possible Bug in RegexURLResolver

2016-07-11 Thread Cristiano Coelho
Wouldn't a standard Lock do the trick? Also you are still vulnerable to a race condition when reading self._populating, if the goal is to avoid populating the object more than once in a short interval (in a case where multiple requests hit the server before the object is initialized for the fir

Re: Possible Bug in RegexURLResolver

2016-07-11 Thread Cristiano Coelho
Sorry for my above answer I think the actual locks should go like this, so all threads can actually go to the wait line while _populating is True. def _populate(self): self.lock.acquire() if self._populating: self.lock.wait() self.lock.release() return

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Aymeric Augustin
Hello, Can you check the condition inside the lock? The following pattern seems simpler to me: def _populate(self): with self._lock: if self._populated: return # … populate … self._populated = True You can look at Apps.populate for an example of this patt

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Cristiano Coelho
Indeed that code is much simpler as long as the changed behaviour of the populated/populating flag doesn't break anything. El martes, 12 de julio de 2016, 4:25:37 (UTC-3), Aymeric Augustin escribió: > > Hello, > > Can you check the condition inside the lock? The following pattern seems > simpler

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Aymeric Augustin
> On 12 Jul 2016, at 14:23, Cristiano Coelho wrote: > > Indeed that code is much simpler as long as the changed behaviour of the > populated/populating flag doesn't break anything. I didn’t check the details; most likely you can reuse the structure but you need to adjust the variable names and

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Aron Griffis
On Tue, Jul 12, 2016 at 3:25 AM Aymeric Augustin < aymeric.augus...@polytechnique.org> wrote: > def _populate(self): > with self._lock: > if self._populated: > return > # … populate … > self._populated = True > Depending on how frequently the code is called

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Florian Apolloner
On Tuesday, July 12, 2016 at 9:25:37 AM UTC+2, Aymeric Augustin wrote: > > Can you check the condition inside the lock? The following pattern seems > simpler to me: > The standard pattern in such cases is to check inside and outside. Outside to avoid the lock if you already populated (the majo

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Aymeric Augustin
> On 12 Jul 2016, at 19:46, Florian Apolloner wrote: > > On Tuesday, July 12, 2016 at 9:25:37 AM UTC+2, Aymeric Augustin wrote: > Can you check the condition inside the lock? The following pattern seems > simpler to me: > > The standard pattern in such cases is to check inside and outside. Outs

Re: Possible Bug in RegexURLResolver

2016-07-12 Thread Cristiano Coelho
Keep in mind your code guys is semantically different from the one of the first post. In the first post, the _populate method can be called more than once, and if the time window is long enough, the two or more calls will eventually run the # ... populate code again, but on your code, the popul

Re: Possible Bug in RegexURLResolver

2016-07-14 Thread Marten Kenbeek
It's not quite as simple. Concurrency is actually not the main issue here. The issue is that self._populate() only populates the app_dict, namespace_dict and reverse_dict for the currently active language. By short-circuiting if the resolver is populating, you have the chance to short-circuit wh

Re: Possible Bug in RegexURLResolver

2016-07-14 Thread Cristiano Coelho
If you are using locals it means each thread will always end up calling the actual populate code? Is there any point for the RegexURLResolver class to be a singleton then? El jueves, 14 de julio de 2016, 9:12:54 (UTC-3), Marten Kenbeek escribió: > > It's not quite as simple. Concurrency is actu

Re: Possible Bug in RegexURLResolver

2016-07-14 Thread Marten Kenbeek
Using a singleton means that everything is cached for the lifetime of the process after the resolver has been populated. The thread-local is so that concurrent calls to _populate() can correctly determine if it is a recursive call within the current thread. _populate() is called *at most* once

Re: Possible Bug in RegexURLResolver

2016-07-14 Thread Markus Holtermann
Thanks everybody! While Aymeric's sounded great, your reasoning sounds even better, Marten. Thanks! Do you want to provide a PR for that, Marten? Cheers, /Markus On Thu, Jul 14, 2016 at 06:47:15AM -0700, Marten Kenbeek wrote: Using a singleton means that everything is cached for the lifetime