Possible Bug in RegexURLResolver

[a.br...@rataran.com]

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.

[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

>--
>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-develop...@googlegroups.com.
>To post to this group, send email to django-d...@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.

[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()

[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()

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

--
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-develop...@googlegroups.com.
To post to this group, send email to django-d...@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.

[Cristiano Coelho]

Indeed that code is much simpler as long as the changed behaviour of the populated/populating flag doesn't break anything.

[Aymeric Augustin]

> On 12 Jul 2016, at 14:23, Cristiano Coelho <cristia...@gmail.com> 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.

[Aron Griffis]

On Tue, Jul 12, 2016 at 3:25 AM Aymeric Augustin <aymeric....@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

[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…

[Aymeric Augustin]

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

-- 

Aymeric.

[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

[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

[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?

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

[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*

>--
>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-develop...@googlegroups.com.
>To post to this group, send email to django-d...@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/4fd29a71-0c2f-4772-afff-8736b68c4e1f%40googlegroups.com.