From cb40d8b1a5eb6dd01e3972db5e1a05b672202f80 Mon Sep 17 00:00:00 2001 From: Frank Tang Date: Thu, 9 May 2019 16:34:24 -0700 Subject: [PATCH] ICU-20595 Make icu::TimeZone::AdoptDefault thread safe correct the mutex Remove comments about not thread safe --- icu4c/source/i18n/timezone.cpp | 23 ++++++++++----- icu4c/source/i18n/unicode/timezone.h | 6 ---- icu4c/source/test/intltest/tzfmttst.cpp | 38 ++++++++++++++++++++++++- icu4c/source/test/intltest/tzfmttst.h | 2 ++ 4 files changed, 55 insertions(+), 14 deletions(-) diff --git a/icu4c/source/i18n/timezone.cpp b/icu4c/source/i18n/timezone.cpp index f129d8b6076..38e6d64132d 100644 --- a/icu4c/source/i18n/timezone.cpp +++ b/icu4c/source/i18n/timezone.cpp @@ -527,6 +527,11 @@ TimeZone::detectHostTimeZone() // ------------------------------------- +static UMutex *gDefaultZoneMutex() { + static UMutex* m = STATIC_NEW(UMutex); + return m; +} + /** * Initialize DEFAULT_ZONE from the system default time zone. * Upon return, DEFAULT_ZONE will not be NULL, unless operator new() @@ -536,6 +541,7 @@ static void U_CALLCONV initDefault() { ucln_i18n_registerCleanup(UCLN_I18N_TIMEZONE, timeZone_cleanup); + Mutex lock(gDefaultZoneMutex()); // If setDefault() has already been called we can skip getting the // default zone information from the system. if (DEFAULT_ZONE != NULL) { @@ -557,9 +563,6 @@ static void U_CALLCONV initDefault() TimeZone *default_zone = TimeZone::detectHostTimeZone(); - // The only way for DEFAULT_ZONE to be non-null at this point is if the user - // made a thread-unsafe call to setDefault() or adoptDefault() in another - // thread while this thread was doing something that required getting the default. U_ASSERT(DEFAULT_ZONE == NULL); DEFAULT_ZONE = default_zone; @@ -571,7 +574,10 @@ TimeZone* U_EXPORT2 TimeZone::createDefault() { umtx_initOnce(gDefaultZoneInitOnce, initDefault); - return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->clone() : NULL; + { + Mutex lock(gDefaultZoneMutex()); + return (DEFAULT_ZONE != NULL) ? DEFAULT_ZONE->clone() : NULL; + } } // ------------------------------------- @@ -581,9 +587,12 @@ TimeZone::adoptDefault(TimeZone* zone) { if (zone != NULL) { - TimeZone *old = DEFAULT_ZONE; - DEFAULT_ZONE = zone; - delete old; + { + Mutex lock(gDefaultZoneMutex()); + TimeZone *old = DEFAULT_ZONE; + DEFAULT_ZONE = zone; + delete old; + } ucln_i18n_registerCleanup(UCLN_I18N_TIMEZONE, timeZone_cleanup); } } diff --git a/icu4c/source/i18n/unicode/timezone.h b/icu4c/source/i18n/unicode/timezone.h index de558f91ac9..ede0c4896df 100644 --- a/icu4c/source/i18n/unicode/timezone.h +++ b/icu4c/source/i18n/unicode/timezone.h @@ -321,10 +321,6 @@ public: * zone is set to the default host time zone. This call adopts the TimeZone object * passed in; the client is no longer responsible for deleting it. * - *

This function is not thread safe. It is an error for multiple threads - * to concurrently attempt to set the default time zone, or for any thread - * to attempt to reference the default zone while another thread is setting it. - * * @param zone A pointer to the new TimeZone object to use as the default. * @stable ICU 2.0 */ @@ -335,8 +331,6 @@ public: * Same as adoptDefault(), except that the TimeZone object passed in is NOT adopted; * the caller remains responsible for deleting it. * - *

See the thread safety note under adoptDefault(). - * * @param zone The given timezone. * @system * @stable ICU 2.0 diff --git a/icu4c/source/test/intltest/tzfmttst.cpp b/icu4c/source/test/intltest/tzfmttst.cpp index 46a640ae688..056ab6cee47 100644 --- a/icu4c/source/test/intltest/tzfmttst.cpp +++ b/icu4c/source/test/intltest/tzfmttst.cpp @@ -85,6 +85,7 @@ TimeZoneFormatTest::runIndexedTest( int32_t index, UBool exec, const char* &name TESTCASE(5, TestFormatTZDBNames); TESTCASE(6, TestFormatCustomZone); TESTCASE(7, TestFormatTZDBNamesAllZoneCoverage); + TESTCASE(8, TestAdoptDefaultThreadSafe); default: name = ""; break; } } @@ -711,6 +712,42 @@ void TimeZoneFormatTest::RunTimeRoundTripTests(int32_t threadNumber) { delete tzids; } +void +TimeZoneFormatTest::TestAdoptDefaultThreadSafe(void) { + ThreadPool threads(this, threadCount, &TimeZoneFormatTest::RunAdoptDefaultThreadSafeTests); + threads.start(); // Start all threads. + threads.join(); // Wait for all threads to finish. +} + +static const int32_t kAdoptDefaultIteration = 10; +static const int32_t kCreateDefaultIteration = 5000; +static const int64_t kStartTime = 1557288964845; + +void TimeZoneFormatTest::RunAdoptDefaultThreadSafeTests(int32_t threadNumber) { + UErrorCode status = U_ZERO_ERROR; + if (threadNumber % 2 == 0) { + for (int32_t i = 0; i < kAdoptDefaultIteration; i++) { + std::unique_ptr timezones( + icu::TimeZone::createEnumeration()); + while (const icu::UnicodeString* timezone = timezones->snext(status)) { + status = U_ZERO_ERROR; + icu::TimeZone::adoptDefault(icu::TimeZone::createTimeZone(*timezone)); + } + } + } else { + int32_t rawOffset; + int32_t dstOffset; + int64_t date = kStartTime; + for (int32_t i = 0; i < kCreateDefaultIteration; i++) { + date += 6000 * i; + std::unique_ptr tz(icu::TimeZone::createDefault()); + status = U_ZERO_ERROR; + tz->getOffset(date, TRUE, rawOffset, dstOffset, status); + status = U_ZERO_ERROR; + tz->getOffset(date, FALSE, rawOffset, dstOffset, status); + } + } +} typedef struct { const char* text; @@ -1301,5 +1338,4 @@ TimeZoneFormatTest::TestFormatTZDBNamesAllZoneCoverage(void) { } } } - #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/test/intltest/tzfmttst.h b/icu4c/source/test/intltest/tzfmttst.h index 89ece5e2e06..1335a087c2f 100644 --- a/icu4c/source/test/intltest/tzfmttst.h +++ b/icu4c/source/test/intltest/tzfmttst.h @@ -29,8 +29,10 @@ class TimeZoneFormatTest : public IntlTest { void TestFormatTZDBNames(void); void TestFormatCustomZone(void); void TestFormatTZDBNamesAllZoneCoverage(void); + void TestAdoptDefaultThreadSafe(void); void RunTimeRoundTripTests(int32_t threadNumber); + void RunAdoptDefaultThreadSafeTests(int32_t threadNumber); }; #endif /* #if !UCONFIG_NO_FORMATTING */