You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@stdcxx.apache.org by Farid Zaripov <Fa...@epam.com> on 2007/09/14 19:16:06 UTC
[PATCH] std::messages thread safety
The 22.locale.messages.cpp test fails due to using incorrect
guard type in functions from messages.cpp file. There used
_RWSTD_MT_STATIC_GUARD(), so that the functions are protected
from working simultaneously with itself only. But from every that
functions invoked __rw_manage_cat_data() which working with
shared global repository of open catalogs.
The proposed patch:
ChangeLog:
* messages.cpp (__rw_cat_open): Use _RWSTD_MT_CLASS_GUARD instead of
_RWSTD_MT_STATIC_GUARD to synchronize acces to global repository of
open
catalogs.
(__rw_get_message): Ditto.
(__rw_get_locale): Ditto.
(__rw_cat_close): Ditto.
Index: messages.cpp
===================================================================
--- messages.cpp (revision 575597)
+++ messages.cpp (working copy)
@@ -213,7 +213,7 @@
int
__rw_cat_open (const _STD::string &cat_name, const _STD::locale &loc)
{
- _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
+ _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
const nl_catd catd = catopen (cat_name.c_str (), NL_CAT_LOCALE);
if (_RWSTD_BAD_CATD == catd)
@@ -239,7 +239,7 @@
if (cat < 0)
return 0;
- _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
+ _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
__rw_open_cat_data *const pcat_data = __rw_manage_cat_data (cat,
0);
@@ -264,7 +264,7 @@
const _STD::locale&
__rw_get_locale (int cat)
{
- _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
+ _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
_RWSTD_ASSERT (0 <= cat);
__rw_open_cat_data* const pcat_data = __rw_manage_cat_data (cat,
0);
@@ -279,7 +279,7 @@
void
__rw_cat_close (int cat)
{
- _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
+ _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
__rw_open_cat_data* const pcat_data =
cat < 0 ? 0 : __rw_manage_cat_data (cat, 0);
Farid.
Re: [PATCH] std::messages thread safety
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
> Martin Sebor wrote:
>> Travis Vitek wrote:
>>> Martin Sebor wrote:
>>>
>>>> AFAIK, catopen() and catgets() are not required to be thread
>>>> safe so we need to guard calls to them across all instances of
>>>> the facet.
>>>>
>>> Yes, but the affected functions are doing more than just calling
>>> catgets() and catopen(). Since they are using shared data, they
>>> need to lock that shared data with a shared mutex.
>> And unless I'm missing something, they are.
>>
>> _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
>>
>> This provides mutual exclusion across all othe such guards
>> (i.e, those with the same type as an argument).
>>
>
> Nope. See _defs.h...
Ouch! You're right. My bad for misremembering, and for misnaming
the macros in the first place.
I withdraw my objection to the patch :)
Martin
>
> // synchronizes access by all objects holding the same mutex
> # define _RWSTD_MT_GUARD(mutex) \
> _RW::__rw_guard _RWSTD_PASTE (__guard, __LINE__) (mutex)
>
> // synchronizes access by all threads holding the same mutex
> # define _RWSTD_MT_STATIC_GUARD(type) \
> typedef _RW::__rw_type<type,__LINE__> _UniqueType; \
> _RWSTD_MT_CLASS_GUARD(_UniqueType)
>
> // synchronizes access by all objects of the same type
> # define _RWSTD_MT_CLASS_GUARD(type) \
> _RWSTD_MT_GUARD (_RW::__rw_get_static_mutex ((type*)0))
>
> _RWSTD_MT_CLASS_GUARD locks a static mutex that unique for the type
> provided. This provides the behavior that you are describing above.
>
> _RWSTD_MT_STATIC_GUARD creates a unique type given a type and a line
> number. That new type is then used to get a handle to a static mutex.
> The only time two _RWSTD_MT_STATIC_GUARD lines are guarding the same
> mutex is if they happen to be on the same line number and have the
> same type argument.
>
> Travis
RE: [PATCH] std::messages thread safety
Posted by Travis Vitek <tv...@quovadx.com>.
Martin Sebor wrote:
>Travis Vitek wrote:
>> Martin Sebor wrote:
>>
>>> AFAIK, catopen() and catgets() are not required to be thread
>>> safe so we need to guard calls to them across all instances of
>>> the facet.
>>>
>>
>> Yes, but the affected functions are doing more than just calling
>> catgets() and catopen(). Since they are using shared data, they
>> need to lock that shared data with a shared mutex.
>
>And unless I'm missing something, they are.
>
> _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
>
>This provides mutual exclusion across all othe such guards
>(i.e, those with the same type as an argument).
>
Nope. See _defs.h...
// synchronizes access by all objects holding the same mutex
# define _RWSTD_MT_GUARD(mutex) \
_RW::__rw_guard _RWSTD_PASTE (__guard, __LINE__) (mutex)
// synchronizes access by all threads holding the same mutex
# define _RWSTD_MT_STATIC_GUARD(type) \
typedef _RW::__rw_type<type,__LINE__> _UniqueType; \
_RWSTD_MT_CLASS_GUARD(_UniqueType)
// synchronizes access by all objects of the same type
# define _RWSTD_MT_CLASS_GUARD(type) \
_RWSTD_MT_GUARD (_RW::__rw_get_static_mutex ((type*)0))
_RWSTD_MT_CLASS_GUARD locks a static mutex that unique for the type
provided. This provides the behavior that you are describing above.
_RWSTD_MT_STATIC_GUARD creates a unique type given a type and a line
number. That new type is then used to get a handle to a static mutex.
The only time two _RWSTD_MT_STATIC_GUARD lines are guarding the same
mutex is if they happen to be on the same line number and have the
same type argument.
Travis
Re: [PATCH] std::messages thread safety
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
> Martin Sebor wrote:
>> Farid Zaripov wrote:
>>> The 22.locale.messages.cpp test fails due to using incorrect
>>> guard type in functions from messages.cpp file. There used
>>> _RWSTD_MT_STATIC_GUARD(), so that the functions are protected
>>> from working simultaneously with itself only. But from every that
>>> functions invoked __rw_manage_cat_data() which working with
>>> shared global repository of open catalogs.
>> _RWSTD_MT_STATIC_GUARD() lock a static local mutex, which guards
>> the rest of the block from being entered by more than 1 thread
>> across the whole process.
>
> Yes, that is the problem. The current static guard prevents multiple
> threads from entering the same function simultaneously [i.e. two threads
> cannot call __rw_cat_open simultaneously], but it does nothing to
> prevent one thread from calling __rw_cat_open() and another thread from
> calling __rw_cat_close() at the same time.
How come? Aren't all the calls guarded using the same mutex?
_RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
This provides mutual exclusion across all othe such guards
(i.e, those with the same type as an argument).
>Since __rw_manage_cat_data
> has shared data, access to it must be synchronized using one common
> lock.
>
>> AFAIK, catopen() and catgets() are not required to be thread
>> safe so we need to guard calls to them across all instances of
>> the facet.
>>
>
> Yes, but the affected functions are doing more than just calling
> catgets() and catopen(). Since they are using shared data, they need to
> lock that shared data with a shared mutex.
And unless I'm missing something, they are.
Martin
>
>> * messages.cpp (__rw_cat_open): Use _RWSTD_MT_CLASS_GUARD instead of
>> _RWSTD_MT_STATIC_GUARD to synchronize acces to global repository of
>> open
>> catalogs.
>> (__rw_get_message): Ditto.
>> (__rw_get_locale): Ditto.
>> (__rw_cat_close): Ditto.
>> Martin
>
RE: [PATCH] std::messages thread safety
Posted by Travis Vitek <tv...@quovadx.com>.
Martin Sebor wrote:
>
>Farid Zaripov wrote:
>>
>> The 22.locale.messages.cpp test fails due to using incorrect
>> guard type in functions from messages.cpp file. There used
>> _RWSTD_MT_STATIC_GUARD(), so that the functions are protected
>> from working simultaneously with itself only. But from every that
>> functions invoked __rw_manage_cat_data() which working with
>> shared global repository of open catalogs.
>
>_RWSTD_MT_STATIC_GUARD() lock a static local mutex, which guards
>the rest of the block from being entered by more than 1 thread
>across the whole process.
Yes, that is the problem. The current static guard prevents multiple
threads from entering the same function simultaneously [i.e. two threads
cannot call __rw_cat_open simultaneously], but it does nothing to
prevent one thread from calling __rw_cat_open() and another thread from
calling __rw_cat_close() at the same time. Since __rw_manage_cat_data
has shared data, access to it must be synchronized using one common
lock.
>AFAIK, catopen() and catgets() are not required to be thread
>safe so we need to guard calls to them across all instances of
>the facet.
>
Yes, but the affected functions are doing more than just calling
catgets() and catopen(). Since they are using shared data, they need to
lock that shared data with a shared mutex.
> * messages.cpp (__rw_cat_open): Use _RWSTD_MT_CLASS_GUARD instead of
> _RWSTD_MT_STATIC_GUARD to synchronize acces to global repository of
> open
> catalogs.
> (__rw_get_message): Ditto.
> (__rw_get_locale): Ditto.
> (__rw_cat_close): Ditto.
>Martin
Re: [PATCH] std::messages thread safety
Posted by Martin Sebor <se...@roguewave.com>.
Farid Zaripov wrote:
> The 22.locale.messages.cpp test fails due to using incorrect
> guard type in functions from messages.cpp file. There used
> _RWSTD_MT_STATIC_GUARD(), so that the functions are protected
> from working simultaneously with itself only. But from every that
> functions invoked __rw_manage_cat_data() which working with
> shared global repository of open catalogs.
_RWSTD_MT_STATIC_GUARD() lock a static local mutex, which guards
the rest of the block from being entered by more than 1 thread
across the whole process. AFAIK, catopen() and catgets() are
not required to be thread safe so we need to guard calls to
them across all instances of the facet. Maybe the problem is
somewhere else? With the static mutex maybe?
Martin
>
> The proposed patch:
>
> ChangeLog:
> * messages.cpp (__rw_cat_open): Use _RWSTD_MT_CLASS_GUARD instead of
> _RWSTD_MT_STATIC_GUARD to synchronize acces to global repository of
> open
> catalogs.
> (__rw_get_message): Ditto.
> (__rw_get_locale): Ditto.
> (__rw_cat_close): Ditto.
>
> Index: messages.cpp
> ===================================================================
> --- messages.cpp (revision 575597)
> +++ messages.cpp (working copy)
> @@ -213,7 +213,7 @@
> int
> __rw_cat_open (const _STD::string &cat_name, const _STD::locale &loc)
> {
> - _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
> + _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
>
> const nl_catd catd = catopen (cat_name.c_str (), NL_CAT_LOCALE);
> if (_RWSTD_BAD_CATD == catd)
> @@ -239,7 +239,7 @@
> if (cat < 0)
> return 0;
>
> - _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
> + _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
>
> __rw_open_cat_data *const pcat_data = __rw_manage_cat_data (cat,
> 0);
>
> @@ -264,7 +264,7 @@
> const _STD::locale&
> __rw_get_locale (int cat)
> {
> - _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
> + _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
>
> _RWSTD_ASSERT (0 <= cat);
> __rw_open_cat_data* const pcat_data = __rw_manage_cat_data (cat,
> 0);
> @@ -279,7 +279,7 @@
> void
> __rw_cat_close (int cat)
> {
> - _RWSTD_MT_STATIC_GUARD (__rw_open_cat_data);
> + _RWSTD_MT_CLASS_GUARD (__rw_open_cat_data);
>
> __rw_open_cat_data* const pcat_data =
> cat < 0 ? 0 : __rw_manage_cat_data (cat, 0);
>
> Farid.