You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rb...@hyperreal.org on 1999/10/06 08:48:52 UTC
cvs commit: apache-2.0/src/lib/apr/time/unix time.c
rbb 99/10/05 23:48:52
Modified: src/lib/apr acconfig.h configure.in
src/lib/apr/include apr_config.h.in apr_errno.h
src/lib/apr/time/unix time.c
Log:
Make time functions threadsafe in APR. This is the current way to make
a function thread-safe, when the C Run-Time function it relies on is NOT
thread-safe. For more information, please read the message that will be
posted to new-httpd@apache.org regarding this topic.
Revision Changes Path
1.3 +34 -0 apache-2.0/src/lib/apr/acconfig.h
Index: acconfig.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/acconfig.h,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- acconfig.h 1999/08/19 13:31:08 1.2
+++ acconfig.h 1999/10/06 06:48:49 1.3
@@ -66,5 +66,39 @@
Sigfunc *signal(int signo, Sigfunc * func);
#endif
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#define SAFETY_LOCK(func_name, cnt, name_str) \
+ { \
+ struct lock_t *funclock = lock_##func_name; \
+ if (funclock == NULL) \
+ if (ap_create_lock(cnt, APR_MUTEX, APR_LOCKALL, name_str, &funclock) != APR_SUCCESS) \
+ return APR_NOTTHREADSAFE; \
+ if (ap_lock(funclock) != APR_SUCCESS) \
+ return APR_NOTTHREADSAFE; \
+ }
+#else
+#define SAFETY_LOCK(func_name, cnt)
+#endif
+
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#define SAFETY_UNLOCK(func_name) \
+ if (ap_unlock(lock_##func_name) != APR_SUCCESS) { \
+ return APR_NOTTHREADSAFE; \
+ }
+#else
+#define SAFETY_UNLOCK(func_name, cnt)
+#endif
+
+#ifdef HAVE_GMTIME_R
+#define GMTIME_R(x, y) gmtime_r(x, y)
+#else
+#define GMTIME_R(x, y) memcpy(y, gmtime(x), sizeof(y))
+#endif
+
+#ifdef HAVE_LOCALTIME_R
+#define LOCALTIME_R(x, y) localtime_r(x, y)
+#else
+#define LOCALTIME_R(x, y) memcpy(y, localtime(x), sizeof(y))
+#endif
#endif /* APR_CONFIG_H */
1.14 +2 -0 apache-2.0/src/lib/apr/configure.in
Index: configure.in
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/configure.in,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- configure.in 1999/10/01 09:47:08 1.13
+++ configure.in 1999/10/06 06:48:49 1.14
@@ -198,6 +198,8 @@
AC_CHECK_FUNCS(getpass)
AC_CHECK_FUNC(_getch)
+AC_CHECK_FUNCS(gmtime_r localtime_r)
+
dnl Start building stuff from our information
AC_SUBST(LDLIBS)
AC_SUBST(OPTIM)
1.8 +40 -0 apache-2.0/src/lib/apr/include/apr_config.h.in
Index: apr_config.h.in
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/include/apr_config.h.in,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -r1.7 -r1.8
--- apr_config.h.in 1999/09/12 12:12:00 1.7
+++ apr_config.h.in 1999/10/06 06:48:50 1.8
@@ -103,6 +103,12 @@
/* Define if you have the getpass function. */
#undef HAVE_GETPASS
+/* Define if you have the gmtime_r function. */
+#undef HAVE_GMTIME_R
+
+/* Define if you have the localtime_r function. */
+#undef HAVE_LOCALTIME_R
+
/* Define if you have the poll function. */
#undef HAVE_POLL
@@ -317,5 +323,39 @@
Sigfunc *signal(int signo, Sigfunc * func);
#endif
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#define SAFETY_LOCK(func_name, cnt, name_str) \
+ { \
+ struct lock_t *funclock = lock_##func_name; \
+ if (funclock == NULL) \
+ if (ap_create_lock(cnt, APR_MUTEX, APR_LOCKALL, name_str, &funclock) != APR_SUCCESS) \
+ return APR_NOTTHREADSAFE; \
+ if (ap_lock(funclock) != APR_SUCCESS) \
+ return APR_NOTTHREADSAFE; \
+ }
+#else
+#define SAFETY_LOCK(func_name, cnt)
+#endif
+
+#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+#define SAFETY_UNLOCK(func_name) \
+ if (ap_unlock(lock_##func_name) != APR_SUCCESS) { \
+ return APR_NOTTHREADSAFE; \
+ }
+#else
+#define SAFETY_UNLOCK(func_name, cnt)
+#endif
+
+#ifdef HAVE_GMTIME_R
+#define GMTIME_R(x, y) gmtime_r(x, y)
+#else
+#define GMTIME_R(x, y) memcpy(y, gmtime(x), sizeof(y))
+#endif
+
+#ifdef HAVE_LOCALTIME_R
+#define LOCALTIME_R(x, y) localtime_r(x, y)
+#else
+#define LOCALTIME_R(x, y) memcpy(y, localtime(x), sizeof(y))
+#endif
#endif /* APR_CONFIG_H */
1.3 +1 -0 apache-2.0/src/lib/apr/include/apr_errno.h
Index: apr_errno.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/include/apr_errno.h,v
retrieving revision 1.2
retrieving revision 1.3
diff -u -r1.2 -r1.3
--- apr_errno.h 1999/09/04 00:21:15 1.2
+++ apr_errno.h 1999/10/06 06:48:51 1.3
@@ -396,6 +396,7 @@
#define APR_ENOSOCKET 4011
#define APR_ENOTHREAD 4012
#define APR_ENOTHDKEY 4013
+#define APR_NOTTHREADSAFE 4014
/* APR STATUS VALUES */
#define APR_INCHILD 5001
1.5 +10 -2 apache-2.0/src/lib/apr/time/unix/time.c
Index: time.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/time/unix/time.c,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- time.c 1999/10/04 16:37:46 1.4
+++ time.c 1999/10/06 06:48:52 1.5
@@ -62,6 +62,10 @@
#include <errno.h>
#include <string.h>
+static ap_lock_t *lock_gmtime = NULL;
+static ap_lock_t *lock_localtime = NULL;
+
+
/* ***APRDOC********************************************************
* ap_status_t ap_make_time(ap_context_t *, ap_time_t *)
* Create a time entity.
@@ -107,11 +111,15 @@
{
switch (type) {
case APR_LOCALTIME: {
- atime->explodedtime = localtime(&atime->currtime->tv_sec);
+ SAFETY_LOCK(localtime, atime->cntxt, "localtimefile");
+ LOCALTIME_R(&atime->currtime->tv_sec, atime->explodedtime);
+ SAFETY_UNLOCK(localtime);
break;
}
case APR_UTCTIME: {
- atime->explodedtime = gmtime(&atime->currtime->tv_sec);
+ SAFETY_LOCK(gmtime, atime->cntxt, "gmtimefile");
+ GMTIME_R(&atime->currtime->tv_sec, atime->explodedtime);
+ SAFETY_UNLOCK(gmtime);
break;
}
}
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Ryan Bloom <rb...@raleigh.ibm.com>.
> But, there is one other problem I just noticed, which is that the lock
> is created in the memory context of the first ap_time_t that uses
> SAFETY_LOCK. If that context is destroyed, the lock goes away. The
> lock has to be in a persistant location (NULL context?).
This cannot be helped. We could use the ap_palloc(NULL, trick but that
will be going away as soon as we all agree on how memory allocation should
be done in APR. The correct solution, would be to allocate these during
the ap_initialize function that is required in all APR functions. The
only problem, is that I want to be able to specify that certain APR
modules aren't to be compiled in. For example, if Apache decides to do
it's own signals work (which I think we have), then when Apache compiles
APR the signaling module should not be included. I haven't put this
ability into the configure script yet, but I will be doing it soon. When
I do, that will complicate matters if I have lock creation in
ap_initialize. The answer to the problem is easy, but the fix that goes
along with that answer is harder. I'll hopefully get to it soon, but it
has to come after the parameter re-order, the Thread-safe fixes, and the
process work that is currently being done. I will not make any promises
on when that will be. In the mean-time, this will work with a minimal
performance hit.
>
> And why are we using a cross-process lock?
Because I am an idiot and wasn't paying attention. Fixed sometime today.
:)
> Ralf brought this up as a concern. It appears that there are two
> different defines that indicate the existence of the _r functions. If
> the _r functions exist on a system with the old define
> (_POSIX_REENTRANT_FUNCTIONS) but not the new one, then they may be
> from an old spec with different definitions. I don't know (and web
> searches have come up with nothing), which is why I'm asking.
I still see this as a broken platform. However, the framework as it
currently stands, will handle this case with few modifications. I do not
want to deal with this case right now, because ot makes a lot of the
macros VERY ugly. If we find out we need this change, we can make it
then.
Ryan
_______________________________________________________________________
Ryan Bloom rbb@raleigh.ibm.com
4205 S Miami Blvd
RTP, NC 27709 It's a beautiful sight to see good dancers
doing simple steps. It's a painful sight to
see beginners doing complicated patterns.
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Manoj Kasichainula <ma...@io.com>.
On Wed, Oct 06, 1999 at 05:46:32PM -0400, Ryan Bloom wrote:
> That's fine. The idea is still valid, and is still the correct one.
You mean the idea of the macros used to provide the locking instead of
doing it directly? Sure.
But, there is one other problem I just noticed, which is that the lock
is created in the memory context of the first ap_time_t that uses
SAFETY_LOCK. If that context is destroyed, the lock goes away. The
lock has to be in a persistant location (NULL context?).
And why are we using a cross-process lock?
> > Also, can we assume that if there are nonstandard _r functions (i.e.
> > _POSIX_THREAD_SAFE_FUNCTIONS is not defined) that they will behave the
> > same as pthreads/SUS dictate? If not, then we should check for
> > _POSIX_THREAD_SAFE_FUNCTIONS instead of letting autoconf check for
> > each _r function individually.
>
> I don't see this as an issue. If a system has a function that is defined
> by a standard, and the function doesn't work according to the standard,
> the platform is broken. Do you know of any such system? Or, is this
> hand-waving over the issue?
Ralf brought this up as a concern. It appears that there are two
different defines that indicate the existence of the _r functions. If
the _r functions exist on a system with the old define
(_POSIX_REENTRANT_FUNCTIONS) but not the new one, then they may be
from an old spec with different definitions. I don't know (and web
searches have come up with nothing), which is why I'm asking.
--
Manoj Kasichainula - manojk at io dot com - http://www.io.com/~manojk/
"Tandems are good if you need hardware which sucks reliably, 24x365."
-- Malcolm Ray.
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Ryan Bloom <rb...@raleigh.ibm.com>.
> According to Single Unix, any of asctime, ctime, getdate,
> gettimeofday, and gmtime can overwrite each other's values. So a
> per-function lock isn't enough in this case.
>
That's fine. The idea is still valid, and is still the correct one. In
this case, we can only protect the functions that APR functions may call,
but there is no way around this. Of course, this shouldn't be a real
problem anyway, because as Ben pointed out, a threaded system which
doesn't have a real threaded library, is a broken system. Anyway, the
only change, is instead of calling it with the actual function name,
change everything to use "timefuncs".
> Also, can we assume that if there are nonstandard _r functions (i.e.
> _POSIX_THREAD_SAFE_FUNCTIONS is not defined) that they will behave the
> same as pthreads/SUS dictate? If not, then we should check for
> _POSIX_THREAD_SAFE_FUNCTIONS instead of letting autoconf check for
> each _r function individually.
I don't see this as an issue. If a system has a function that is defined
by a standard, and the function doesn't work according to the standard,
the platform is broken. Do you know of any such system? Or, is this
hand-waving over the issue?
Ryan
_______________________________________________________________________
Ryan Bloom rbb@raleigh.ibm.com
4205 S Miami Blvd
RTP, NC 27709 It's a beautiful sight to see good dancers
doing simple steps. It's a painful sight to
see beginners doing complicated patterns.
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Manoj Kasichainula <ma...@raleigh.ibm.com>.
On Wed, Oct 06, 1999 at 06:48:52AM -0000, rbb@hyperreal.org wrote:
> +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
> +#define SAFETY_LOCK(func_name, cnt, name_str) \
> + { \
> + struct lock_t *funclock = lock_##func_name; \
> + if (funclock == NULL) \
> + if (ap_create_lock(cnt, APR_MUTEX, APR_LOCKALL, name_str, &funclock) != APR_SUCCESS) \
> + return APR_NOTTHREADSAFE; \
> + if (ap_lock(funclock) != APR_SUCCESS) \
> + return APR_NOTTHREADSAFE; \
> + }
According to Single Unix, any of asctime, ctime, getdate,
gettimeofday, and gmtime can overwrite each other's values. So a
per-function lock isn't enough in this case.
Also, can we assume that if there are nonstandard _r functions (i.e.
_POSIX_THREAD_SAFE_FUNCTIONS is not defined) that they will behave the
same as pthreads/SUS dictate? If not, then we should check for
_POSIX_THREAD_SAFE_FUNCTIONS instead of letting autoconf check for
each _r function individually.
--
Manoj Kasichainula - manojk@raleigh.ibm.com
IBM, Apache Development
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Manoj Kasichainula <ma...@raleigh.ibm.com>.
On Thu, Oct 07, 1999 at 03:05:12PM -0400, Ryan Bloom wrote:
> The reason for getting rid of configure, was because the configure
> script is created different ways on different platforms. My
> understanding from talking to other developers, is that the
> autoheader doesn't have that issue. It generates the same .h.in
> file everytime, assuming the configure.in script hasn't changed.
Consistency (all auto-generated files are not included in CVS) and
simplification of commit process (no CVS commits require rerunning
code generators and committing their changes).
--
Manoj Kasichainula - manojk@raleigh.ibm.com
IBM, Apache Development
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Rasmus Lerdorf <ra...@raleigh.ibm.com>.
> I have no objections, but why do we want to get rid of it? The reason for
> getting rid of configure, was because the configure script is created
> different ways on different platforms. My understanding from talking to
> other developers, is that the autoheader doesn't have that issue. It
> generates the same .h.in file everytime, assuming the configure.in script
> hasn't changed.
>
> So, I'm -0 for getting rid of it, just because I don't see the point in
> not having it.
One reason would be to avoid questions like yours where people end up
adding symbols to the wrong file. It is much harder to see which file is
generated if we distribute both.
-Rasmus
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Ryan Bloom <rb...@raleigh.ibm.com>.
On Thu, 7 Oct 1999, Manoj Kasichainula wrote:
> On Thu, Oct 07, 1999 at 08:56:37AM -0400, Ryan Bloom wrote:
> > > Also, why are these macros in two places? acconfig.h and apr_config.h.in
> >
> > Because apr_config.h.in is a generated file from autoheader.
>
> Then we should delete it from the CVS repository like we deleted the
> generated stuff from autoconf. autoheader is part of the autoconf
> package, so this won't put any added burden on developers.
>
> Any objections?
I have no objections, but why do we want to get rid of it? The reason for
getting rid of configure, was because the configure script is created
different ways on different platforms. My understanding from talking to
other developers, is that the autoheader doesn't have that issue. It
generates the same .h.in file everytime, assuming the configure.in script
hasn't changed.
So, I'm -0 for getting rid of it, just because I don't see the point in
not having it.
Ryan
_______________________________________________________________________
Ryan Bloom rbb@raleigh.ibm.com
4205 S Miami Blvd
RTP, NC 27709 It's a beautiful sight to see good dancers
doing simple steps. It's a painful sight to
see beginners doing complicated patterns.
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Manoj Kasichainula <ma...@io.com>.
On Thu, Oct 07, 1999 at 08:56:37AM -0400, Ryan Bloom wrote:
> > Also, why are these macros in two places? acconfig.h and apr_config.h.in
>
> Because apr_config.h.in is a generated file from autoheader.
Then we should delete it from the CVS repository like we deleted the
generated stuff from autoconf. autoheader is part of the autoconf
package, so this won't put any added burden on developers.
Any objections?
--
Manoj Kasichainula - manojk at io dot com - http://www.io.com/~manojk/
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Ryan Bloom <rb...@raleigh.ibm.com>.
On Wed, 6 Oct 1999, Greg Stein wrote:
> Euh... it looks like this never stores a value back into lock_##func_name.
> Therefore, it will create a lock on every entry to the function.
Good catch. This is the problem with making a lot of revisions to code, I
needed the re-direction one point, but now it's a bug. Thanks, I'll fix
this ASAP.
>
> Also, why are these macros in two places? acconfig.h and apr_config.h.in
Because apr_config.h.in is a generated file from autoheader. It takes the
def from acconfig, and just throws it into apr_config.h.in. It is
actually a really nice combination with autoheader and autoconf together.
I can still control what goes into apr_config.h.in, but if I add a new
function check in configure.in, autoheader finds it for me automatically.
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
>
>
>
_______________________________________________________________________
Ryan Bloom rbb@raleigh.ibm.com
4205 S Miami Blvd
RTP, NC 27709 It's a beautiful sight to see good dancers
doing simple steps. It's a painful sight to
see beginners doing complicated patterns.
Re: cvs commit: apache-2.0/src/lib/apr/time/unix time.c
Posted by Greg Stein <gs...@lyra.org>.
On 6 Oct 1999 rbb@hyperreal.org wrote:
>...
> +#ifndef _POSIX_THREAD_SAFE_FUNCTIONS
> +#define SAFETY_LOCK(func_name, cnt, name_str) \
> + { \
> + struct lock_t *funclock = lock_##func_name; \
> + if (funclock == NULL) \
> + if (ap_create_lock(cnt, APR_MUTEX, APR_LOCKALL, name_str, &funclock) != APR_SUCCESS) \
> + return APR_NOTTHREADSAFE; \
> + if (ap_lock(funclock) != APR_SUCCESS) \
> + return APR_NOTTHREADSAFE; \
> + }
Euh... it looks like this never stores a value back into lock_##func_name.
Therefore, it will create a lock on every entry to the function.
Also, why are these macros in two places? acconfig.h and apr_config.h.in
Cheers,
-g
--
Greg Stein, http://www.lyra.org/