Re: FreeBSD compile problem (was Getting ready for 3.2 beta 2)

[Gregory (Grisha) Trubetskoy]

I'd keep the patch nonetheless - I think this is how it should be.

Grisha

On Mon, 12 Sep 2005, Jim Gallacher wrote:

-1 for this patch. Actually, the patch itself is fine - it just doesn't fix the problem. The unit tests are still failings as per my previous messages. ie the following is getting logged in test/logs/error_log:

[Mon Sep 12 19:49:33 2005] [emerg] (2)No such file or directory: Couldn't create accept lock

Jim

Gregory (Grisha) Trubetskoy wrote:

Here's a patch (this is against 3.1.2b). Untested. This replaces GIL with with the APR lock.

--- src/mod_python.c.orig       Mon Sep 12 16:42:28 2005
+++ src/mod_python.c    Mon Sep 12 17:32:26 2005
@@ -31,7 +31,9 @@
  * (In a Python dictionary) */
 static PyObject * interpreters = NULL;

+#ifdef APR_HAS_THREAD
 static apr_thread_mutex_t* interpreters_lock = 0;
+#endif

 apr_pool_t *child_init_pool = NULL;

@@ -127,9 +129,8 @@
     if (! name)
         name = MAIN_INTERPRETER;

-#ifdef WITH_THREAD
+#ifdef APR_HAS_THREAD
     apr_thread_mutex_lock(interpreters_lock);
-    PyEval_AcquireLock();
 #endif

     if (!interpreters) {
@@ -156,8 +157,7 @@
         idata = (interpreterdata *)PyCObject_AsVoidPtr(p);
     }

-#ifdef WITH_THREAD
-    PyEval_ReleaseLock();
+#ifdef APR_HAS_THREAD
     apr_thread_mutex_unlock(interpreters_lock);
 #endif

@@ -513,8 +513,10 @@
         /* initialze the interpreter */
         Py_Initialize();

-#ifdef WITH_THREAD
+#ifdef APR_HAS_THREAD
         
apr_thread_mutex_create(&interpreters_lock,APR_THREAD_MUTEX_UNNESTED,p);
+#endif
+#ifdef WITH_THREAD
         /* create and acquire the interpreter lock */
         PyEval_InitThreads();
 #endif




On Mon, 12 Sep 2005, Gregory (Grisha) Trubetskoy wrote:

Yep, this is getting a little hairy, but nothing we couldn't handle :-)

I did a little more research. Basically, this started with Graham's patch that addressed a problem with modules being reimported (or something). From Graham's message:

The basic problem revolves around the Python dictionary used to hold the set of interpreters. The code in mod_python.c is trying to use the Python GIL to provide exclusive access to that dictionary and any subsequent creation of an interpreter.

The only catch is that in creating a new interpreter, the Python core is, in someway I don't understand, swapping thread states at some point which is allowing other threads to acquire the GIL.

So what Graham's patch does is create an APR lock (interpreters_lock) and wrap all the access to the dictionary with calls to apr_mutex_lock/unlock.

I think the _real_ way to address this issue is to first find what is the problem with using the Python GIL to serialize access to the interpreters dictionary. Is this a Python bug, or are we not understanding GIL and using it improperly?

BUT, given that the above question may be complicated to answer, and that Graham's patch resolves the issue, another thought:

If the APR lock works, what is the point of using the GIL in addition? Should we just use the APR-based lock alone? I.e., where we had (after Graham's patch):

#ifdef WITH_THREAD
     apr_thread_mutex_lock(interpreters_lock);
     PyEval_AcquireLock();
#endif

we would use:

 #ifdef APR_HAS_THREAD
     apr_thread_mutex_lock(interpreters_lock);
 #endif

_without_ a call to PyEval_AcquireLock() at all.

It should compile OK, and on platforms where APR has no thread support, like you said, it's not an issue since no separate interpreters run in one process at the same time.

Grisha

On Mon, 12 Sep 2005, Nicolas Lehuen wrote:

Duh, this is becoming difficult :)

I was thinking that if APR_HAS_THREADS was 0, then Apache was forcibly ran in prefork mode, so there was no need for thread safety at all, given the

fact that mod_python would only run one interpreter thread. So if

WITH_THREAD was not defined, ORAPR_HAS_THREADS was 0, then we would not need

any thread safety code. Hence the definition of
MOD_PYTHON_WITH_THREAD_SUPPORT.

You're right in writing that a user could launch a new thread in Python, provided that WITH_THREAD is defined, even if APR_HAS_THREADS==0. However, having a look at the parts of mod_python.c where the thread safety was put in, I think we can safely say that those parts are only called by mod_python (through python_handler, python_cleanup etc who call get_interpreter). Those parts are therefore always called in the same thread (if APR_HAS_THREADS==0, that is) and there is no need for thread synchronization to be done (no shared data between the main thread and the other user threads, no need to

release the GIL etc.).

BUT, I could be very, very wrong here, and your idea of reverting to a
conservative "shield python threading calls with WITH_THREAD and apr

threading code with APR_HAS_THREADS" is way more attractive to my tired mind

right now. So if you want I can revert all this
MOD_PYTHON_WITH_THREAD_SUPPORT hack.

Regards,
Nicolas

2005/9/12, Gregory (Grisha) Trubetskoy <[EMAIL PROTECTED]>:

I'm not sure I understand this, perhaps someone could write a message to the list explaining what we're doing here so there is a record. Sorry if

I'm being slow-headed here.

To me it seems that when you use thread-related calls from Python, you wrap those in Python defines (WITH_THREAD) and when you use thread-related calls from APR, you wrap those in APR defines (APR_HAS_THREAD), and that's

all?

In other words - what does MOD_PYTHON_WITH_THREAD_SUPPORT accomplish that

the above does not.

Also, given:

#if(defined(WITH_THREAD) && APR_HAS_THREADS)
#define MOD_PYTHON_WITH_THREAD_SUPPORT 1
#else
#define MOD_PYTHON_WITH_THREAD_SUPPORT 0
#endif

Does this mean that if Python is compiled with thread support and APR is

not, MOD_PYTHON_WITH_THREAD_SUPPORT is 0 which means that the thread

safety code isn't there, but you still _can_ create threads because Python

will let you - isn't this asking for a segfault/deadlock/whatever?

Grisha

On Mon, 12 Sep 2005, Jim Gallacher wrote:

Gregory (Grisha) Trubetskoy wrote:

Shouldn't that be PYTHON_WITH_THREAD rather than

MOD_PYTHON_WITH_THREAD?

I understand it to mean that we want the thread handling code compiled

into

mod_python.

Compiling and testing right now.

Jim

On Mon, 12 Sep 2005, Nicolas Lehuen wrote:

I've checked in a changeset wherein I define
MOD_PYTHON_WITH_THREAD_SUPPORT

and use it everywhere WITH_THREAD was previously used. This should do

the

trick ! Now if someone (like Jim) can give us his +1, that would be

great.

Regards,
Nicolas

2005/9/12, Gregory (Grisha) Trubetskoy <[EMAIL PROTECTED]>:

Just wanted to add to this message that if Jim's version runs and

tests

with the trick below (envvars is executed prior to apache start, but

I

don't think the tests use it, so you'll probably just have to set

this

var

in the shell in which the tests are run), then this would be a

solution

for all FreeBSD issues and we could roll a beta 3 which will have a

great
change of being publicly released.

Grisha

On Mon, 12 Sep 2005, Gregory (Grisha) Trubetskoy wrote:

OK, found it. This should work on FreeBSD where Python is threaded

and

Apache

is not.

[snip]

And, if you built apache without thread support, you may need to add

the
following lines to $PREFIX/sbin/envvars:

LD_PRELOAD=/usr/lib/libc_r.so
export LD_PRELOAD

[snip]


On Mon, 12 Sep 2005, Gregory (Grisha) Trubetskoy wrote:

On Mon, 12 Sep 2005, Jim Gallacher wrote:

*** Warning: Linking the shared library mod_python.la against

the
*** static library
/usr/local/lib/python2.4/config/libpython2.4.a is

not

portable!

I think this was always there and its pretty harmless.

On qemu:
Syntax error on line 44 of
/usr/home/jim/tmp/mod_python/test/conf/test.conf:

Cannot load /usr/home/jim/tmp/mod_python/src/mod_python.so into

server:

/usr/home/jim/tmp/mod_python/src/mod_python.so: Undefined symbol

"pthread_attr_init"

This is because FreeBSD's libc comes in two versions - threaded

and

non-threaded. If Python is linked against the threaded ones and

Apache

against the non-thrreaded, then you get this problem. There is a

simple

fix for this - you just cause Apache to start with threaded libs,

but I

can't find any references to it right now and have to run off to a

meeting.

Grisha

It is quite possible I don't have things configured correctly on

the

QEMU version and hence the different undefined symbol but it

doesn't

really matter since it fails either way. I don't have time to investigate further right now. I'll revisit this tonight.

Regards,
Jim

Regards,
Nicolas
#if APR_HAS_THREADS && defined(WITH_THREAD)
2005/9/11, Jim Gallacher <[EMAIL PROTECTED]
<mailto:[EMAIL PROTECTED]>>:

FYI, I found the following note in the INSTALL file in the

apache
source:

* If you are building on FreeBSD, be aware that threads will be disabled and the prefork MPM will be used by default, as threads do not work well with Apache on FreeBSD. If you wish to try a threaded Apache on FreeBSD anyway, use

"./configure --enable-threads".

I'm also setting up FreeBSD under QEMU... so far so good, but

installing

anything using ports is really slow. QEMU's performance here

is
just

killing me. I guess I should have read the manual first and

used
the

binary packages for the software I wanted to install. :-(

Regards,
Jim

Jim Gallacher wrote:

Nicolas Lehuen wrote:

OK, I've checked in a version that compiles both on at

least

Win32 and

FreeBSD. I'm just testing if APR_HAS_THREAD is defined and

only

include the apr_thread_mutex_lock and unlock calls if it

is

defined.

Compiles a passes unit tests on Linux Debian sid with

mpm-prefork.

Now, on minotaur, APR_HAS_THREAD is defined as 0. Does

this

mean
that

Apache is not configured for threading ? Can we assume

that we

are in

the prefork model if APR_HAS_THREAD==0, so that we can

skip

all the

locking code ? Because that's what we do right now.

On Debian sid with apache2.0.54 mpm-prefork, APR_HAS_THREAD

==

1.

Jim

Regards,
Nicolas

2005/9/11, Nicolas Lehuen <[EMAIL PROTECTED]

<mailto:[EMAIL PROTECTED]>

<mailto: [EMAIL PROTECTED]

<mailto:[EMAIL PROTECTED]>>>:

Yes, this new code is something I commited on the

29/12/2004
(I used

the "blame" function of TortoiseSVN for that). It was a

patch by

Graham to fix MODPYTHON-2
<http://issues.apache.org/jira/browse/MODPYTHON-2>.

The problem is not in the patch, but rather in the fact

that
APR

seems configured without the thread support while Python

is

configured with thread support. mod_python.c assumes that

is

WITH_THREAD is defined, then the APR mutex functions are

available,

which is wrong. Maybe we should test for APR_HAS_THREADS

instead ?

In that case, won't this cause any problems on threaded

platforms ?

I don't know if this is a problem specific to minotaur or

to
all

version of FreeBSD. I'm currently downloading the ISOs of

FreeBSD

and I'll try using QEMU to run a FreeBSD setup on my

computer, but

that will be long and troublesome. If someone has more

clue
on this

issue, feel free to tell us :).

Regards,
Nicolas

2005/9/10, Jim Gallacher <[EMAIL PROTECTED]

<mailto:[EMAIL PROTECTED]>

<mailto:[EMAIL PROTECTED]

<mailto:[EMAIL PROTECTED]>>>:

I'm stubling around in the dark here, but maybe this

will

create a

spark

of an idea. I took a diff of mod_python.c from 3.1.4 and

3.2.1b and

isolated the lines which correspond to the compilation

error.

Compiler messages
-----------------

mod_python.c:34: error: syntax error before '*' token mod_python.c:34: warning: type defaults to `int' in

declaration of

`interpreters_lock'

mod_python.c:34: warning: data definition has no type or

storage class

mod_python.c: In function `get_interpreter':

mod_python.c:131: warning: implicit declaration of

function
`apr_thread_mutex_lock'

mod_python.c:161: warning: implicit declaration of

function
`apr_thread_mutex_unlock'
mod_python.c: In function `python_init':

mod_python.c:517: warning: implicit declaration of

function
`apr_thread_mutex_create'

mod_python.c:517: error: `APR_THREAD_MUTEX_UNNESTED'

undeclared (first

use in this function)


Diff output
-----------

I've only copied the diff chunks which correspond to the

complier

errors

mentioned above.

--- mod_python-3.1.4/src/mod_python.c Sat Jan 29

13:25:28

2005

+++ mod_python-3.2.1b/src/mod_python.c Tue Sep 6

17:11:03

2005

@@ -31,6 +31,8 @@
* (In a Python dictionary) */
static PyObject * interpreters = NULL;

+static apr_thread_mutex_t* interpreters_lock = 0;

+
apr_pool_t *child_init_pool = NULL;

... snip ...

@@ -124,11 +128,15 @@
name = MAIN_INTERPRETER;

#ifdef WITH_THREAD
+ apr_thread_mutex_lock(interpreters_lock);
PyEval_AcquireLock();
#endif

... snip ...

@@ -149,6 +158,7 @@

#ifdef WITH_THREAD
PyEval_ReleaseLock();
+ apr_thread_mutex_unlock(interpreters_lock);
#endif

... snip ...

@@ -490,13 +506,15 @@
}

/* initialize global Python interpreter if necessary */

- if (! Py_IsInitialized())
+ if (initialized == 0 || !Py_IsInitialized())
{
-
+ initialized = 1;
+
/* initialze the interpreter */
Py_Initialize();

#ifdef WITH_THREAD
+

apr_thread_mutex_create(&interpreters_lock,APR_THREAD_MUTEX_UNNESTED,p);

/* create and acquire the interpreter lock */
PyEval_InitThreads();
#endif

So it would seem that the code causing the compile

problems

is new

for 3.2.

I also notice that in apr_arch_thread_mutex.h the

typedef

for

apr_thread_mutex_t is wrapped by #if APR_HAS_THREADS /

#endif.

Looking at the apache source in

srclib/apr/locks/unix/thread_mutex.c,

everything is also enclosed by #if APR_HAS_THREADS /

#endif.

eg, apr_thread_mutex_create, apr_thread_mutex_lock and

apr_thread_mutex_unlock.

Hopefully this will give someone a clue as to what may

be

going on

here

with FreeBSD.

Regards,
Jim