See Branko's reaction for why the atomic handling will be necessary when you
don't want to hardcode hardware assumptions. Bugs in these assumptions are
exactly the problem we try to solve here.
This implementation idea is copied from how Microsoft implemented some of
their thunking in a completely thread safe way, with the least amount of
overhead possible after the first call. You don't even need function
inlining this way for an optimal call.
And to your third point: The function pointer itself is the inline function
with this implementation.
Your suggestion is like:
// A already defined as variable
static APR_INLINE int A()
{
return A();
}
Luckily you just get a compiler error for the duplicated symbol, otherwise
it would just implement endless recursion instead of an actual function
call.
Bert
From: daniel.lescohier_at_cbsinteractive.com
[mailto:daniel.lescohier_at_cbsinteractive.com] On Behalf Of Daniel Lescohier
Sent: zaterdag 7 december 2013 16:05
To: Bert Huijben
Cc: William A. Rowe Jr.; Stefan Fuhrmann; APR Developer List; Stefan
Fuhrman; Philip Martin; Subversion Development
Subject: Re: Race condition in APR_DECLARE_LATE_DLL_FUNC() implementation
I don't think the static function pointer should be declared volatile. That
would mean that for the normal use of the function pointer in the main part
of the program, compiler optimizations of memory address load reordering
would be disabled. It's only operations that require strict memory ordering
that should declare it volatile. That is why all the apr_atomic functions
"cast" values through a volatile pointer.
Second: why use apr_atomic_casptr? Writing a pointer to a memory address is
already atomic: another thread cannot see a halfway-changed pointer. What
the apr_atomic_casptr would prevent would it from being written twice with
the new address value. But why do we need to prevent that? I think it's
all right to have a race to write the new function pointer; it's all right
if it's written multiple times, since they are all writing the same value.
Third: missing is the declaration of apr_winapi_##fn, referenced later in
the file, which should be:
static APR_INLINE rettype apr_winapi_##fn args \
{ return apr_winapi_##fn names; } /* call the function pointer */
On Sat, Dec 7, 2013 at 5:51 AM, Bert Huijben <bert_at_qqmail.nl
<mailto:bert_at_qqmail.nl> > wrote:
> -----Original Message-----
> From: Bert Huijben [mailto:bert_at_qqmail.nl <mailto:bert_at_qqmail.nl> ]
> Sent: vrijdag 6 december 2013 19:14
> To: 'William A. Rowe Jr.'; 'Stefan Fuhrmann'
> Cc: 'APR Developer List'; 'Stefan Fuhrman'; 'Philip Martin'; 'Subversion
> Development'
> Subject: RE: Race condition in APR_DECLARE_LATE_DLL_FUNC()
> implementation
>
>
>
> > -----Original Message-----
> > From: William A. Rowe Jr. [mailto:wrowe_at_rowe-clan.net
<mailto:wrowe_at_rowe-clan.net> ]
> > Sent: vrijdag 6 december 2013 18:24
> > To: Stefan Fuhrmann
> > Cc: Bert Huijben; APR Developer List; Stefan Fuhrman; Philip Martin;
> > Subversion Development
> > Subject: Re: Race condition in APR_DECLARE_LATE_DLL_FUNC()
> > implementation
> >
> > On Fri, 6 Dec 2013 16:44:52 +0100
> > Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com
<mailto:stefan.fuhrmann_at_wandisco.com> > wrote:
> >
> > > On Fri, Dec 6, 2013 at 6:05 AM, William A. Rowe Jr.
> > > <wrowe_at_rowe-clan.net <mailto:wrowe_at_rowe-clan.net> >wrote:
> > >
> > > > On Thu, 5 Dec 2013 15:01:05 +0100
> > > > "Bert Huijben" <bert_at_qqmail.nl <mailto:bert_at_qqmail.nl> > wrote:
> > > >
> > > > > I think the dll load function should be converted to a more stable
> > > > > pattern, that properly handles multiple threads. And perhaps we
> > > > > should just assume a few more NT functions to be alsways there
> > > > > instead of loading them dynamically.
> > > >
> > > > This is possible with the 'mandatory' call to apr_init, but I think
> > > > the existing pattern should persist for those who don't like to
> > > > call the initialization logic.
> > > >
> > >
> > > We currently call apr_initialize() before spawning threads or doing
> > > anything other APR. What else do we need to become thread-safe
> > > under Windows?
> >
> > Working on a patch. Simply, we just need an _initialize hack that
> > triggers this lookup for each internally required (or not-present) fn.
>
> It is also possible to rewrite the functions to do an atomic replace of
the
> function pointer, which makes them safe for multithreaded usage. (Same
> pattern as we use in Subversion for atomic initializations or even simpler
> variants as it is safe to do the same LoadLibraryXX() and GetProcAddress()
> in multiple threads at once, via the Windows loader lock.)
>
> But as far as I can see all the functions are always available on Windows
XP
> and later (and the others are not even used by APR and APR-Util), so I'm
not
> really sure if we should really spend more time on this.
A patch like the following would make the code thread safe.
[[
Index: include/arch/win32/apr_arch_misc.h
===================================================================
--- include/arch/win32/apr_arch_misc.h (revision 1547134)
+++ include/arch/win32/apr_arch_misc.h (working copy)
@@ -18,6 +18,7 @@
#define MISC_H
#include "apr.h"
+#include "apr_atomic.h"
#include "apr_portable.h"
#include "apr_private.h"
#include "apr_general.h"
@@ -188,22 +189,23 @@
* ERROR_INVALID_FUNCTION if the function cannot be loaded
*/
#define APR_DECLARE_LATE_DLL_FUNC(lib, rettype, calltype, fn, ord, args,
names) \
- typedef rettype (calltype *apr_winapi_fpt_##fn) args; \
- static apr_winapi_fpt_##fn apr_winapi_pfn_##fn = NULL; \
- static int apr_winapi_chk_##fn = 0; \
- static APR_INLINE int apr_winapi_ld_##fn(void) \
- { if (apr_winapi_pfn_##fn) return 1; \
- if (apr_winapi_chk_##fn ++) return 0; \
- if (!apr_winapi_pfn_##fn) \
- apr_winapi_pfn_##fn = (apr_winapi_fpt_##fn) \
- apr_load_dll_func(lib, #fn, ord); \
- if (apr_winapi_pfn_##fn) return 1; else return 0; }; \
- static APR_INLINE rettype apr_winapi_##fn args \
- { if (apr_winapi_ld_##fn()) \
- return (*(apr_winapi_pfn_##fn)) names; \
- else { SetLastError(ERROR_INVALID_FUNCTION); return 0;} }; \
+ typedef rettype (calltype *apr_winapi_fpt_##fn) args;
\
+ static rettype calltype apr_winapi_ld_##fn args;
\
+ static volatile apr_winapi_fpt_##fn apr_winapi_##fn =
apr_winapi_ld_##fn; \
+ static rettype calltype apr_winapi_unavail_##fn args
\
+ { SetLastError(ERROR_INVALID_FUNCTION); return 0; }
\
+ static rettype calltype apr_winapi_ld_##fn args
\
+ {
\
+ apr_winapi_fpt_##fn dl_func = (apr_winapi_fpt_##fn)
\
+ apr_load_dll_func(lib, #fn, ord);
\
+ if (! dl_func)
\
+ dl_func = apr_winapi_unavail_##fn;
\
+ apr_atomic_casptr((volatile void**)&apr_winapi_##fn, dl_func,
\
+ apr_winapi_ld_##fn);
\
+ return apr_winapi_##fn names;
\
+ }
-#define APR_HAVE_LATE_DLL_FUNC(fn) apr_winapi_ld_##fn()
+/*#define APR_HAVE_LATE_DLL_FUNC(fn) apr_winapi_ld_##fn() */
/* Provide late bound declarations of every API function missing from
* one or more supported releases of the Win32 API
]]
It is not a complete solution as it breaks APR_HAVE_LATE_DLL_FUNC().
The only caller of this macro uses it to check for the existence of
WsaPoll(). This should just be checked with the Windows version/feature
level like how we check all others functions in APR, or better yet the usage
of WsaPoll should just be removed for the know issues this function has.
See http://daniel.haxx.se/blog/2012/10/10/wsapoll-is-broken/ for a summary
of the problems and Microsoft's explanation that they are not going to fix
this 'for compatibility reasons'.
In all our uses in Subversion and Serf we have explicit code to *not* use
WsaPoll via apr for these reasons.
If we use it the network IO will just get stuck on network problems.
>
> Bert
Received on 2013-12-11 13:05:26 CET