Alright, cool. So I guess we're ready to roll the next tarfile now.
Grisha
On Tue, 13 Sep 2005, Nicolas Lehuen wrote:
I'm sure it is required, even after fixing the S in APR_HAS_THREADS I tried
with and without the PyEval_AcquireLock code and the latter works while the
former doesn't. I don't know why though... I'll have to review the code once
more to understand.
Regards,
Nicolas
2005/9/13, Gregory (Grisha) Trubetskoy <[EMAIL PROTECTED]>:
Good job - I've tested it and it works on FreeBSD. Just one last thing I
wanted to confirm - are you sure that PyEval_AcquireLock is required, or
was this a sideffect of the missing 'S'? I just want to make sure we're
not doing double locking where it's not needed.
Grisha
On Tue, 13 Sep 2005, Nicolas Lehuen wrote:
OK I fixed the problem, please check again on FreeBSD. Here is what I've
done (from the subversion comment) :
"Fixed : APR_HAS_THREADS ends with an 'S'. Reintroduced the calls to
PyEval_AcquireLock and PyEval_ReleaseLock as they are required if
threading
is enabled. Removed two debugging log entries. Added the conditional
commenting of the LockFile directive in the LockFile class."
Regards,
Nicolas
2005/9/13, Nicolas Lehuen <[EMAIL PROTECTED]>:
Well, bad news... I have a -1 for now on Win32. I don't know why, but
when
I install and test the 20050911 version, everything is OK. When I
install
and test the latest version, the unit test behave very strangely (with
or
without the changes done by Graham). Basically, the Apache server takes
forever to stop. Weird...
I'm trying to find out what happens here.
Regards,
Nicolas
2005/9/13, Jim Gallacher <[EMAIL PROTECTED]>:
Gregory (Grisha) Trubetskoy wrote:
Done. All tests pass on my FreeBSD box. Nicolas - can you test Win32,
I'm not 100% sure if the change to test.py I made will work.
Good news. If the changes can be checked in and Nicolas can give a +1
on
the Windows test then I'll be able to generate the next, and hopefully
last, beta tonight.
As an aside, it may be useful to have an option for test.py to
preserve
a copy of the apache logs. It would make troubleshooting the unit test
failures much easier. Currently test.py deletes the logs after each
unit
test.
Regards,
Jim
Grisha
On Tue, 13 Sep 2005, Nicolas Lehuen wrote:
Yes, now I remember I had to comment this line out because it broke
something on Win32. Go ahead, uncomment it and I'll add a test to
remove it
when running the test on Win32.
Regards,
Nicolas
2005/9/13, Gregory (Grisha) Trubetskoy < [EMAIL PROTECTED]>:
OK, I found the problem. The LockFile line is commented out in
test.py
which causes Apache to try to create the lock in the default
location in
/var/run.
http://svn.apache.org/viewcvs.cgi/httpd/mod_python/trunk/test/test.py?rev=125771&r1=106619&r2=125771
So the question is - can we just put it back in, or does it break
something on Win32?
Cheers
Grisha
On Tue, 13 Sep 2005, Jim Gallacher wrote:
Nicolas Lehuen wrote:
Jim, do you manage to build and test the 3.1.4 version on your
setup ?
This looks like a permission problem, not something related to
our
current
problem.
I haven't tried 3.1.4. And I could also try the tests as root,
which
would
eliminate any permission problems. I have a busy day ahead of me
so
this
will
have to wait until tonight.
Jim
Regards,
Nicolas
2005/9/13, Jim Gallacher <[EMAIL PROTECTED]
<mailto: [EMAIL PROTECTED]>>:
-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]
<mailto:[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]
<mailto:[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]>
<mailto:[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]>>
<mailto: [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]>>
<mailto: [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
