Code

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#11193 closed (fixed)

WSGI handler not properly handling lock

Reported by: Phillip Sitbon Owned by: nobody
Component: Core (Other) Version: 1.0
Severity: Keywords: threads, locks
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

For this code in wsgi.py, seen in the latest release and the [source:django/trunk/django/core/handlers/wsgi.py@9996#L226 latest repository revision]:

        if self._request_middleware is None:
            self.initLock.acquire()
            # Check that middleware is still uninitialised.
            if self._request_middleware is None:
                self.load_middleware()
            self.initLock.release()

I noticed the issue of thread safety in this handler came up in #10470, but this code is still not thread safe because it it can be put into a deadlock state. It will deadlock after any of the middleware loading fails- for example, when the module pointed to by DJANGO_SETTINGS_MODULE isn't found (which is how I came to notice it). After the failure, initLock is held and so this can deadlock on initLock.acquire().

The fix is very simple, and should absolutely be done anywhere locking is done:

        if self._request_middleware is None:
            self.initLock.acquire()
            try:
                # Check that middleware is still uninitialised.
                if self._request_middleware is None:
                    self.load_middleware()
            finally:
                self.initLock.release()

Of course using with keyword is an option as well. Also, I'm not sure how you would handle an indeterminate state between initialized and not initialized (e.g. some middleware loaded), but it might be a good idea to set self._request_middleware back to None before releasing the lock in case of an exception:

        if self._request_middleware is None:
            self.initLock.acquire()
            try:
                # Check that middleware is still uninitialised.
                if self._request_middleware is None:
                    self.load_middleware()
            except:
                # possibly unload whatever middleware didn't fail here
                self._request_middleware = None
                raise
            finally:
                self.initLock.release()

Attachments (0)

Change History (8)

comment:1 Changed 5 years ago by Phillip Sitbon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Sorry, I forgot to point out a few things:

  • I'm using Django with the latest version of PyISAPIe
  • The code I submitted does indeed fix the deadlock issue for me.

comment:2 Changed 5 years ago by anonymous

  • Version changed from SVN to 1.0

comment:3 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by anonymous

  • milestone set to 1.3

comment:5 Changed 3 years ago by jacob

Oops, that was me.

comment:6 Changed 3 years ago by andrewgodwin

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

(In [15205]) Fixed #11193 -- WSGI handler not properly handling lock on error in load_middleware. Thanks to Phillip Sitbon.

comment:7 Changed 3 years ago by andrewgodwin

(In [15206]) [1.2.X] Fixed #11193 -- WSGI handler not properly handling lock on error in load_middleware. Thanks to Phillip Sitbon.

Backport of [15205] from trunk

comment:8 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.