Re: Possible Bug in RegexURLResolver

[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 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 per thread for each language. If one thread finishes populating before
another thread starts, that second thread can access the thread-independent
cache and won't have to populate the resolver again.

On Thursday, July 14, 2016 at 3:15:43 PM UTC+2, Cristiano Coelho wrote:


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 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 while
the populating thread has a different active language. The short-circuiting
thread's language won't have been populated, and that will result in the
above KeyError.

The issue that self._populating is solving is that a RegexURLResolver can
recursively include itself if a namespace is used. Namespaces are loaded
lazily on first access, so this would generally not result in infinite
recursion. But, to include all namespaced views in self._callback_strs,
you need to load them immediately. self._populating prevents infinite
recursion in that specific case.

On a side note: recursively including the same resolver is possible under
some limited circumstances, but so far I've only seen it happen in the
Django test suite, and it doesn't even work if you don't include at least
one namespace between each recursive include. It's an edge-case scenario
that can be solved better by using a repeating pattern in your regex. I
don't think that it provides any value, but it does significantly
complicate the code. Removing this accidental "feature" would solve the
issue as well, without complicating the code any further.

Now, to solve the issue within the constraints of the current test suite,
you only need to prevent that self._populate() is called twice on the
same object within the same thread. A simple thread-local object should do
the trick:

class RegexURLResolver(LocaleRegexProvider):
def __init__(self, regex, urlconf_name, default_kwargs=None,
app_name=None, namespace=None):

...

self._local = threading.local()
self._local.populating = False
...

   def _populate(self):
if self._local.populating:
return
self._local.populating = True
...
self._local.populating = False


This will work concurrently, because all lists (lookups, namespaces, apps)
are built up in local variables, and then set in
self._namespace_dict[language_code] etc. as an atomic operation. The
worst that can happen is that the list is overwritten atomically with an
identical list. self._callback_strs is a set, so updating it with values
that are already present is not a problem either.


Marten


On Wednesday, July 13, 2016 at 12:22:36 AM UTC+2, Cristiano Coelho wrote:


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
populate code will only be called once doesn't matter how many times you
call _populate, unless the populated variable is restarted somewhere else.
So I don't know how is this exactly used but make sure to check it

El martes, 12 de julio de 2016, 14:58:12 (UTC-3), Aymeric Augustin
escribió:


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.
Outside to avoid the lock if you already populated (the majority of
requests) and inside to see if another thread populated it in the time you
waited to get the lock…


Yes, actually that’s what I did the last time I implemented this
pattern, in Apps.populate.

--
Aymeric.





--
You received this message because you are subscribed to the Google Groups "Django 
developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr.

Re: Possible Bug in RegexURLResolver

[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 per thread for each language. If one thread finishes populating before 
another thread starts, that second thread can access the thread-independent 
cache and won't have to populate the resolver again.

On Thursday, July 14, 2016 at 3:15:43 PM UTC+2, Cristiano Coelho wrote:
>
> 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 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 while 
>> the populating thread has a different active language. The short-circuiting 
>> thread's language won't have been populated, and that will result in the 
>> above KeyError.
>>
>> The issue that self._populating is solving is that a RegexURLResolver can 
>> recursively include itself if a namespace is used. Namespaces are loaded 
>> lazily on first access, so this would generally not result in infinite 
>> recursion. But, to include all namespaced views in self._callback_strs, 
>> you need to load them immediately. self._populating prevents infinite 
>> recursion in that specific case.
>>
>> On a side note: recursively including the same resolver is possible under 
>> some limited circumstances, but so far I've only seen it happen in the 
>> Django test suite, and it doesn't even work if you don't include at least 
>> one namespace between each recursive include. It's an edge-case scenario 
>> that can be solved better by using a repeating pattern in your regex. I 
>> don't think that it provides any value, but it does significantly 
>> complicate the code. Removing this accidental "feature" would solve the 
>> issue as well, without complicating the code any further. 
>>
>> Now, to solve the issue within the constraints of the current test suite, 
>> you only need to prevent that self._populate() is called twice on the 
>> same object within the same thread. A simple thread-local object should do 
>> the trick:
>>
>> class RegexURLResolver(LocaleRegexProvider):
>> def __init__(self, regex, urlconf_name, default_kwargs=None, 
>> app_name=None, namespace=None):
>> 
>> ...
>>
>> self._local = threading.local()
>> self._local.populating = False
>> ...
>>
>>def _populate(self):
>> if self._local.populating:
>> return
>> self._local.populating = True
>> ...
>> self._local.populating = False
>>
>>
>> This will work concurrently, because all lists (lookups, namespaces, apps) 
>> are built up in local variables, and then set in 
>> self._namespace_dict[language_code] etc. as an atomic operation. The 
>> worst that can happen is that the list is overwritten atomically with an 
>> identical list. self._callback_strs is a set, so updating it with values 
>> that are already present is not a problem either.
>>
>>
>> Marten
>>
>>
>> On Wednesday, July 13, 2016 at 12:22:36 AM UTC+2, Cristiano Coelho wrote:
>>>
>>> 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 
>>> populate code will only be called once doesn't matter how many times you 
>>> call _populate, unless the populated variable is restarted somewhere else. 
>>> So I don't know how is this exactly used but make sure to check it
>>>
>>> El martes, 12 de julio de 2016, 14:58:12 (UTC-3), Aymeric Augustin 
>>> escribió:

 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. 
 Outside to avoid the lock if you already populated (the majority of 
 requests) and inside to see if another thread populated it in the time you 
 waited to get the lock…


 Yes, actually that’s what I did the last time I implemented this 
 pattern, in Apps.populate.

 -- 
 Aymeric.




-- 
You received this message because you are subscribed to the Google Groups 
"Dja

Re: Possible Bug in RegexURLResolver

[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 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 while 
> the populating thread has a different active language. The short-circuiting 
> thread's language won't have been populated, and that will result in the 
> above KeyError.
>
> The issue that self._populating is solving is that a RegexURLResolver can 
> recursively include itself if a namespace is used. Namespaces are loaded 
> lazily on first access, so this would generally not result in infinite 
> recursion. But, to include all namespaced views in self._callback_strs, 
> you need to load them immediately. self._populating prevents infinite 
> recursion in that specific case.
>
> On a side note: recursively including the same resolver is possible under 
> some limited circumstances, but so far I've only seen it happen in the 
> Django test suite, and it doesn't even work if you don't include at least 
> one namespace between each recursive include. It's an edge-case scenario 
> that can be solved better by using a repeating pattern in your regex. I 
> don't think that it provides any value, but it does significantly 
> complicate the code. Removing this accidental "feature" would solve the 
> issue as well, without complicating the code any further. 
>
> Now, to solve the issue within the constraints of the current test suite, 
> you only need to prevent that self._populate() is called twice on the 
> same object within the same thread. A simple thread-local object should do 
> the trick:
>
> class RegexURLResolver(LocaleRegexProvider):
> def __init__(self, regex, urlconf_name, default_kwargs=None, 
> app_name=None, namespace=None):
> 
> ...
>
> self._local = threading.local()
> self._local.populating = False
> ...
>
>def _populate(self):
> if self._local.populating:
> return
> self._local.populating = True
> ...
> self._local.populating = False
>
>
> This will work concurrently, because all lists (lookups, namespaces, apps) 
> are built up in local variables, and then set in 
> self._namespace_dict[language_code] etc. as an atomic operation. The 
> worst that can happen is that the list is overwritten atomically with an 
> identical list. self._callback_strs is a set, so updating it with values 
> that are already present is not a problem either.
>
>
> Marten
>
>
> On Wednesday, July 13, 2016 at 12:22:36 AM UTC+2, Cristiano Coelho wrote:
>>
>> 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 populate code will 
>> only be called once doesn't matter how many times you call _populate, 
>> unless the populated variable is restarted somewhere else. So I don't know 
>> how is this exactly used but make sure to check it
>>
>> El martes, 12 de julio de 2016, 14:58:12 (UTC-3), Aymeric Augustin 
>> escribió:
>>>
>>> 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. 
>>> Outside to avoid the lock if you already populated (the majority of 
>>> requests) and inside to see if another thread populated it in the time you 
>>> waited to get the lock…
>>>
>>>
>>> Yes, actually that’s what I did the last time I implemented this 
>>> pattern, in Apps.populate.
>>>
>>> -- 
>>> Aymeric.
>>>
>>>
>>>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/a9788779-8a04-44f1-b18b-e8048f4c6a4f%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 while the 
populating thread has a different active language. The short-circuiting 
thread's language won't have been populated, and that will result in the 
above KeyError.

The issue that self._populating is solving is that a RegexURLResolver can 
recursively include itself if a namespace is used. Namespaces are loaded 
lazily on first access, so this would generally not result in infinite 
recursion. But, to include all namespaced views in self._callback_strs, you 
need to load them immediately. self._populating prevents infinite recursion 
in that specific case.

On a side note: recursively including the same resolver is possible under 
some limited circumstances, but so far I've only seen it happen in the 
Django test suite, and it doesn't even work if you don't include at least 
one namespace between each recursive include. It's an edge-case scenario 
that can be solved better by using a repeating pattern in your regex. I 
don't think that it provides any value, but it does significantly 
complicate the code. Removing this accidental "feature" would solve the 
issue as well, without complicating the code any further. 

Now, to solve the issue within the constraints of the current test suite, 
you only need to prevent that self._populate() is called twice on the same 
object within the same thread. A simple thread-local object should do the 
trick:

class RegexURLResolver(LocaleRegexProvider):
def __init__(self, regex, urlconf_name, default_kwargs=None, 
app_name=None, namespace=None):

...

self._local = threading.local()
self._local.populating = False
...

   def _populate(self):
if self._local.populating:
return
self._local.populating = True
...
self._local.populating = False


This will work concurrently, because all lists (lookups, namespaces, apps) 
are built up in local variables, and then set in 
self._namespace_dict[language_code] etc. as an atomic operation. The worst 
that can happen is that the list is overwritten atomically with an 
identical list. self._callback_strs is a set, so updating it with values 
that are already present is not a problem either.


Marten


On Wednesday, July 13, 2016 at 12:22:36 AM UTC+2, Cristiano Coelho wrote:
>
> 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 populate code will 
> only be called once doesn't matter how many times you call _populate, 
> unless the populated variable is restarted somewhere else. So I don't know 
> how is this exactly used but make sure to check it
>
> El martes, 12 de julio de 2016, 14:58:12 (UTC-3), Aymeric Augustin 
> escribió:
>>
>> 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. 
>> Outside to avoid the lock if you already populated (the majority of 
>> requests) and inside to see if another thread populated it in the time you 
>> waited to get the lock…
>>
>>
>> Yes, actually that’s what I did the last time I implemented this pattern, 
>> in Apps.populate.
>>
>> -- 
>> Aymeric.
>>
>>
>>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/dfa519d1-cee3-4ed5-b4ff-a5e902b5310e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 populate code will 
only be called once doesn't matter how many times you call _populate, 
unless the populated variable is restarted somewhere else. So I don't know 
how is this exactly used but make sure to check it

El martes, 12 de julio de 2016, 14:58:12 (UTC-3), Aymeric Augustin escribió:
>
> 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. Outside 
> to avoid the lock if you already populated (the majority of requests) and 
> inside to see if another thread populated it in the time you waited to get 
> the lock…
>
>
> Yes, actually that’s what I did the last time I implemented this pattern, 
> in Apps.populate.
>
> -- 
> Aymeric.
>
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/cfbb3858-cbc7-4d90-b7a7-ca4656f131e9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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. Outside to 
> avoid the lock if you already populated (the majority of requests) and inside 
> to see if another thread populated it in the time you waited to get the lock…

Yes, actually that’s what I did the last time I implemented this pattern, in 
Apps.populate.

-- 
Aymeric.


-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/3DF500DF-F5CA-4862-B088-A86C7C1B6628%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 majority of requests) and 
inside to see if another thread populated it in the time you waited to get 
the lock…

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/e4a55afc-b7d5-4a9b-88d5-2a80cb528756%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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, it might be worthwhile to
check self._populated prior to obtaining the lock.

def _populate(self):
if self._populated:
return
with self._lock:
if self._populated:
return
# ... populate ...
self._populated = True

This way the only time you're waiting on the lock is when you actually need
to be waiting on the lock, i.e. while another thread is populating.

-Aron

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CADu1%3D220Hmiqq-MeH3gSRtW9C2eqmS7A3jZ%2B25dSiPPcsa%3DzVw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 the conditions.

-- 
Aymeric.


-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/860E5D32-CF5E-437A-B306-3E4FBAF114CC%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 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 pattern.
>
> Please forgive me if I missed a reason for using something more 
> complicated.
>
> self._lock needs to be a RLock if _populate can call itself recursively in 
> a given thread.
>
> Best regards,
>
> -- 
> Aymeric.
>
> On 12 Jul 2016, at 03:20, Cristiano Coelho  > wrote:
>
> 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
> 
> self._populating = True
> 
> self.lock.release()
> 
> ...
>
> self.lock.acquire()
> self._populating = False
> self.lock.notify_all()
> self.lock.release()
>
>
>
> El lunes, 11 de julio de 2016, 22:14:04 (UTC-3), Cristiano Coelho escribió:
>>
>> 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 
>> first time?) you are still running all the critical code on all threads if 
>> they check self.populating before it is set to True.
>>
>> Would a condition work better in this case? Something like this.. (add 
>> any missing try/finally/with that might be required), warning, NOT TESTED, 
>> just an idea.
>> The below code will not be prone to race conditions as the variables are 
>> always read/set under a lock, and also the critical section code will be 
>> only executed ONCE even if multiple threads attempt to run it at once, 
>> while still locking all threads to prevent returning before the code is 
>> done.
>>
>> def __init__(self, regex, urlconf_name, default_kwargs=None, app_name=
>> None, namespace=None):
>> 
>> ...
>>
>> self._populating = False
>> self.lock = threading.Condition()
>>
>> def _populate(self):
>>
>> self.lock.acquire()
>> 
>> if self._populating:
>> self.lock.wait()
>> self.lock.release()
>> return
>> 
>> self._populating = True
>>
>> ...
>>
>> self._populating = False
>> self.lock.notify_all()
>> self.lock.release()
>>
>>
>>
>>
>>
>> El lunes, 11 de julio de 2016, 10:08:50 (UTC-3), a.br...@rataran.com 
>> escribió:
>>>
>>> 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/django/urls/resolvers.py", line 
>>> 241, in reverse_dict
>>> return self._reverse_dict[language_code]
>>> KeyError: 'it'
>>>
>>> After a wile I figured out that RegexURLResolver is memoized by 
>>> get_resolver and so it acts like a singleton for a certain number of 
>>> requests.
>>>
>>> Analyzing the code of  RegexURLResolver I found that the method 
>>> _poupulate will return directly if it has been called before and not yet 
>>> finished.
>>>
>>> ...
>>> def _populate(self):
>>> if self._populating:
>>> return
>>> self._populating = True
>>> ...  
>>>
>>> if used for recursive call in a single thread this will not hurt, but in 
>>> my case in uwsgi multi thread mode I got the error.
>>>
>>> here is my quick and dirty fix:
>>>
>>> class RegexURLResolver(LocaleRegexProvider):
>>> def __init__(self, regex, urlconf_name, default_kwargs=None, 
>>> app_name=None, namespace=None):
>>> 
>>> ...
>>>
>>> self._populating = False
>>> self.RLock = threading.RLock()
>>>
>>> ...
>>>
>>>def _populate(self):
>>> if self._populating:
>>> self.RLock.acquire()
>>> self.RLock.release()
>>> return
>>> self._populating = True
>>> self.RLock.acquire()
>>> 
>>> ...
>>>
>>> self._populating = False
>>> self.RLock.release()
>>>
>>>
>>> Does anyone know if there is a better solution?
>>>
>>> Thank you.
>>>
>>
> -- 
> You received this message because you are subscribed to the Google Groups 
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop 

Re: Possible Bug in RegexURLResolver

[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 pattern.

Please forgive me if I missed a reason for using something more complicated.

self._lock needs to be a RLock if _populate can call itself recursively in a 
given thread.

Best regards,

-- 
Aymeric.

> On 12 Jul 2016, at 03:20, Cristiano Coelho  wrote:
> 
> 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
> 
> self._populating = True
> 
> self.lock.release()
> 
> ...
> 
> self.lock.acquire()
> self._populating = False
> self.lock.notify_all()
> self.lock.release()
> 
> 
> 
> El lunes, 11 de julio de 2016, 22:14:04 (UTC-3), Cristiano Coelho escribió:
> 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 
> first time?) you are still running all the critical code on all threads if 
> they check self.populating before it is set to True.
> 
> Would a condition work better in this case? Something like this.. (add any 
> missing try/finally/with that might be required), warning, NOT TESTED, just 
> an idea.
> The below code will not be prone to race conditions as the variables are 
> always read/set under a lock, and also the critical section code will be only 
> executed ONCE even if multiple threads attempt to run it at once, while still 
> locking all threads to prevent returning before the code is done.
> 
> def __init__(self, regex, urlconf_name, default_kwargs=None, app_name=None, 
> namespace=None):
> 
> ...
> 
> self._populating = False
> self.lock = threading.Condition()
> 
> def _populate(self):
> 
> self.lock.acquire()
> 
> if self._populating:
> self.lock.wait()
> self.lock.release()
> return
> 
> self._populating = True
> 
> ...
> 
> self._populating = False
> self.lock.notify_all()
> self.lock.release()
> 
> 
> 
> 
> 
> El lunes, 11 de julio de 2016, 10:08:50 (UTC-3), a.br...@rataran.com <> 
> escribió:
> 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/django/urls/resolvers.py", line 241, 
> in reverse_dict
> return self._reverse_dict[language_code]
> KeyError: 'it'
> 
> After a wile I figured out that RegexURLResolver is memoized by get_resolver 
> and so it acts like a singleton for a certain number of requests.
> 
> Analyzing the code of  RegexURLResolver I found that the method _poupulate 
> will return directly if it has been called before and not yet finished.
> 
> ...
> def _populate(self):
> if self._populating:
> return
> self._populating = True
> ...  
> 
> if used for recursive call in a single thread this will not hurt, but in my 
> case in uwsgi multi thread mode I got the error.
> 
> here is my quick and dirty fix:
> 
> class RegexURLResolver(LocaleRegexProvider):
> def __init__(self, regex, urlconf_name, default_kwargs=None, 
> app_name=None, namespace=None):
> 
> ...
> 
> self._populating = False
> self.RLock = threading.RLock()
> 
> ...
> 
>def _populate(self):
> if self._populating:
> self.RLock.acquire()
> self.RLock.release()
> return
> self._populating = True
> self.RLock.acquire()
> 
> ...
> 
> self._populating = False
> self.RLock.release()
> 
> 
> Does anyone know if there is a better solution?
> 
> Thank you.
> 
> -- 
> You received this message because you are subscribed to the Google Groups 
> "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to django-developers+unsubscr...@googlegroups.com 
> .
> To post to this group, send email to django-developers@googlegroups.com 
> .
> Visit this group at https://groups.google.com/group/django-developers 
> .
>

Re: Possible Bug in RegexURLResolver

[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

self._populating = True

self.lock.release()

...

self.lock.acquire()
self._populating = False
self.lock.notify_all()
self.lock.release()



El lunes, 11 de julio de 2016, 22:14:04 (UTC-3), Cristiano Coelho escribió:
>
> 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 
> first time?) you are still running all the critical code on all threads if 
> they check self.populating before it is set to True.
>
> Would a condition work better in this case? Something like this.. (add any 
> missing try/finally/with that might be required), warning, NOT TESTED, just 
> an idea.
> The below code will not be prone to race conditions as the variables are 
> always read/set under a lock, and also the critical section code will be 
> only executed ONCE even if multiple threads attempt to run it at once, 
> while still locking all threads to prevent returning before the code is 
> done.
>
> def __init__(self, regex, urlconf_name, default_kwargs=None, app_name=None
> , namespace=None):
> 
> ...
>
> self._populating = False
> self.lock = threading.Condition()
>
> def _populate(self):
>
> self.lock.acquire()
> 
> if self._populating:
> self.lock.wait()
> self.lock.release()
> return
> 
> self._populating = True
>
> ...
>
> self._populating = False
> self.lock.notify_all()
> self.lock.release()
>
>
>
>
>
> El lunes, 11 de julio de 2016, 10:08:50 (UTC-3), a.br...@rataran.com 
> escribió:
>>
>> 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/django/urls/resolvers.py", line 
>> 241, in reverse_dict
>> return self._reverse_dict[language_code]
>> KeyError: 'it'
>>
>> After a wile I figured out that RegexURLResolver is memoized by 
>> get_resolver and so it acts like a singleton for a certain number of 
>> requests.
>>
>> Analyzing the code of  RegexURLResolver I found that the method 
>> _poupulate will return directly if it has been called before and not yet 
>> finished.
>>
>> ...
>> def _populate(self):
>> if self._populating:
>> return
>> self._populating = True
>> ...  
>>
>> if used for recursive call in a single thread this will not hurt, but in 
>> my case in uwsgi multi thread mode I got the error.
>>
>> here is my quick and dirty fix:
>>
>> class RegexURLResolver(LocaleRegexProvider):
>> def __init__(self, regex, urlconf_name, default_kwargs=None, 
>> app_name=None, namespace=None):
>> 
>> ...
>>
>> self._populating = False
>> self.RLock = threading.RLock()
>>
>> ...
>>
>>def _populate(self):
>> if self._populating:
>> self.RLock.acquire()
>> self.RLock.release()
>> return
>> self._populating = True
>> self.RLock.acquire()
>> 
>> ...
>>
>> self._populating = False
>> self.RLock.release()
>>
>>
>> Does anyone know if there is a better solution?
>>
>> Thank you.
>>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/922ca727-dd60-41dd-9b78-b8d8fe9ebd16%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 
first time?) you are still running all the critical code on all threads if 
they check self.populating before it is set to True.

Would a condition work better in this case? Something like this.. (add any 
missing try/finally/with that might be required), warning, NOT TESTED, just 
an idea.
The below code will not be prone to race conditions as the variables are 
always read/set under a lock, and also the critical section code will be 
only executed ONCE even if multiple threads attempt to run it at once, 
while still locking all threads to prevent returning before the code is 
done.

def __init__(self, regex, urlconf_name, default_kwargs=None, app_name=None, 
namespace=None):

...

self._populating = False
self.lock = threading.Condition()

def _populate(self):

self.lock.acquire()

if self._populating:
self.lock.wait()
self.lock.release()
return

self._populating = True

...

self._populating = False
self.lock.notify_all()
self.lock.release()





El lunes, 11 de julio de 2016, 10:08:50 (UTC-3), a.br...@rataran.com 
escribió:
>
> 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/django/urls/resolvers.py", line 
> 241, in reverse_dict
> return self._reverse_dict[language_code]
> KeyError: 'it'
>
> After a wile I figured out that RegexURLResolver is memoized by 
> get_resolver and so it acts like a singleton for a certain number of 
> requests.
>
> Analyzing the code of  RegexURLResolver I found that the method _poupulate 
> will return directly if it has been called before and not yet finished.
>
> ...
> def _populate(self):
> if self._populating:
> return
> self._populating = True
> ...  
>
> if used for recursive call in a single thread this will not hurt, but in 
> my case in uwsgi multi thread mode I got the error.
>
> here is my quick and dirty fix:
>
> class RegexURLResolver(LocaleRegexProvider):
> def __init__(self, regex, urlconf_name, default_kwargs=None, 
> app_name=None, namespace=None):
> 
> ...
>
> self._populating = False
> self.RLock = threading.RLock()
>
> ...
>
>def _populate(self):
> if self._populating:
> self.RLock.acquire()
> self.RLock.release()
> return
> self._populating = True
> self.RLock.acquire()
> 
> ...
>
> self._populating = False
> self.RLock.release()
>
>
> Does anyone know if there is a better solution?
>
> Thank you.
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/0782172d-2648-4770-83ff-93dfd00ade17%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Possible Bug in RegexURLResolver

[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 for another issue
https://code.djangoproject.com/ticket/24931

/Markus

On Sun, Jul 10, 2016 at 11:47:35PM -0700, a.bra...@rataran.com wrote:

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/django/urls/resolvers.py", line 241,
in reverse_dict
   return self._reverse_dict[language_code]
KeyError: 'it'

After a wile I figured out that RegexURLResolver is memoized by
get_resolver and so it acts like a singleton for a certain number of
requests.

Analyzing the code of  RegexURLResolver I found that the method _poupulate
will return directly if it has been called before and not yet finished.

   ...
   def _populate(self):
   if self._populating:
   return
   self._populating = True
   ...

if used for recursive call in a single thread this will not hurt, but in my
case in uwsgi multi thread mode I got the error.

here is my quick and dirty fix:

class RegexURLResolver(LocaleRegexProvider):
   def __init__(self, regex, urlconf_name, default_kwargs=None,
app_name=None, namespace=None):

   ...

   self._populating = False
   self.RLock = threading.RLock()

   ...

  def _populate(self):
   if self._populating:
   self.RLock.acquire()
   self.RLock.release()
   return
   self._populating = True
   self.RLock.acquire()

   ...

   self._populating = False
   self.RLock.release()


Does anyone know if there is a better solution?

Thank you.

--
You received this message because you are subscribed to the Google Groups "Django 
developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/308c7d89-27e5-4841-85b1-55b0584cef3b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


--
You received this message because you are subscribed to the Google Groups "Django 
developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/20160711204150.GE11144%40inel.local.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Possible Bug in RegexURLResolver

[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/django/urls/resolvers.py", line 241, 
in reverse_dict
return self._reverse_dict[language_code]
KeyError: 'it'

After a wile I figured out that RegexURLResolver is memoized by 
get_resolver and so it acts like a singleton for a certain number of 
requests.

Analyzing the code of  RegexURLResolver I found that the method _poupulate 
will return directly if it has been called before and not yet finished.

...
def _populate(self):
if self._populating:
return
self._populating = True
...  

if used for recursive call in a single thread this will not hurt, but in my 
case in uwsgi multi thread mode I got the error.

here is my quick and dirty fix:

class RegexURLResolver(LocaleRegexProvider):
def __init__(self, regex, urlconf_name, default_kwargs=None, 
app_name=None, namespace=None):

...

self._populating = False
self.RLock = threading.RLock()

...

   def _populate(self):
if self._populating:
self.RLock.acquire()
self.RLock.release()
return
self._populating = True
self.RLock.acquire()

...

self._populating = False
self.RLock.release()


Does anyone know if there is a better solution?

Thank you.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/308c7d89-27e5-4841-85b1-55b0584cef3b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.