diff --git a/icu4c/source/i18n/dtitvfmt.cpp b/icu4c/source/i18n/dtitvfmt.cpp index 21377e111a0..be3220db083 100644 --- a/icu4c/source/i18n/dtitvfmt.cpp +++ b/icu4c/source/i18n/dtitvfmt.cpp @@ -1,5 +1,5 @@ /******************************************************************************* -* Copyright (C) 2008-2015, International Business Machines Corporation and +* Copyright (C) 2008-2016, International Business Machines Corporation and * others. All Rights Reserved. ******************************************************************************* * @@ -17,20 +17,20 @@ //TODO: put in compilation //#define DTITVFMT_DEBUG 1 -#include "cstring.h" #include "unicode/msgfmt.h" #include "unicode/dtptngen.h" #include "unicode/dtitvinf.h" #include "unicode/calendar.h" + +#include "cstring.h" #include "dtitv_impl.h" +#include "gregoimp.h" +#include "mutex.h" #ifdef DTITVFMT_DEBUG #include -#include "cstring.h" #endif -#include "gregoimp.h" - U_NAMESPACE_BEGIN @@ -63,7 +63,10 @@ static const UChar gEarlierFirstPrefix[] = {LOW_E, LOW_A, LOW_R, LOW_L, LOW_I, L UOBJECT_DEFINE_RTTI_IMPLEMENTATION(DateIntervalFormat) +// Mutex, protects access to fDateFormat, fFromCalendar and fToCalendar. +// Needed because these data members are modified by const methods of DateIntervalFormat. +static UMutex gFormatterMutex = U_MUTEX_INITIALIZER; DateIntervalFormat* U_EXPORT2 DateIntervalFormat::createInstance(const UnicodeString& skeleton, @@ -148,26 +151,29 @@ DateIntervalFormat::operator=(const DateIntervalFormat& itvfmt) { delete fDatePattern; delete fTimePattern; delete fDateTimeFormat; - if ( itvfmt.fDateFormat ) { - fDateFormat = (SimpleDateFormat*)itvfmt.fDateFormat->clone(); - } else { - fDateFormat = NULL; + { + Mutex lock(&gFormatterMutex); + if ( itvfmt.fDateFormat ) { + fDateFormat = (SimpleDateFormat*)itvfmt.fDateFormat->clone(); + } else { + fDateFormat = NULL; + } + if ( itvfmt.fFromCalendar ) { + fFromCalendar = itvfmt.fFromCalendar->clone(); + } else { + fFromCalendar = NULL; + } + if ( itvfmt.fToCalendar ) { + fToCalendar = itvfmt.fToCalendar->clone(); + } else { + fToCalendar = NULL; + } } if ( itvfmt.fInfo ) { fInfo = itvfmt.fInfo->clone(); } else { fInfo = NULL; } - if ( itvfmt.fFromCalendar ) { - fFromCalendar = itvfmt.fFromCalendar->clone(); - } else { - fFromCalendar = NULL; - } - if ( itvfmt.fToCalendar ) { - fToCalendar = itvfmt.fToCalendar->clone(); - } else { - fToCalendar = NULL; - } fSkeleton = itvfmt.fSkeleton; int8_t i; for ( i = 0; i< DateIntervalInfo::kIPI_MAX_INDEX; ++i ) { @@ -201,50 +207,41 @@ DateIntervalFormat::clone(void) const { UBool DateIntervalFormat::operator==(const Format& other) const { - if (typeid(*this) == typeid(other)) { - const DateIntervalFormat* fmt = (DateIntervalFormat*)&other; -#ifdef DTITVFMT_DEBUG - UBool equal; - equal = (this == fmt); - - equal = (*fInfo == *fmt->fInfo); - equal = (*fDateFormat == *fmt->fDateFormat); - equal = fFromCalendar->isEquivalentTo(*fmt->fFromCalendar) ; - equal = fToCalendar->isEquivalentTo(*fmt->fToCalendar) ; - equal = (fSkeleton == fmt->fSkeleton); - equal = ((fDatePattern == NULL && fmt->fDatePattern == NULL) || (fDatePattern && fmt->fDatePattern && *fDatePattern == *fmt->fDatePattern)); - equal = ((fTimePattern == NULL && fmt->fTimePattern == NULL) || (fTimePattern && fmt->fTimePattern && *fTimePattern == *fmt->fTimePattern)); - equal = ((fDateTimeFormat == NULL && fmt->fDateTimeFormat == NULL) || (fDateTimeFormat && fmt->fDateTimeFormat && *fDateTimeFormat == *fmt->fDateTimeFormat)); -#endif - UBool res; - res = ( this == fmt ) || - ( Format::operator==(other) && - fInfo && - ( *fInfo == *fmt->fInfo ) && - fDateFormat && - ( *fDateFormat == *fmt->fDateFormat ) && - fFromCalendar && - fFromCalendar->isEquivalentTo(*fmt->fFromCalendar) && - fToCalendar && - fToCalendar->isEquivalentTo(*fmt->fToCalendar) && - fSkeleton == fmt->fSkeleton && - ((fDatePattern == NULL && fmt->fDatePattern == NULL) || (fDatePattern && fmt->fDatePattern && *fDatePattern == *fmt->fDatePattern)) && - ((fTimePattern == NULL && fmt->fTimePattern == NULL) || (fTimePattern && fmt->fTimePattern && *fTimePattern == *fmt->fTimePattern)) && - ((fDateTimeFormat == NULL && fmt->fDateTimeFormat == NULL) || (fDateTimeFormat && fmt->fDateTimeFormat && *fDateTimeFormat == *fmt->fDateTimeFormat)) && fLocale == fmt->fLocale); - int8_t i; - for (i = 0; i< DateIntervalInfo::kIPI_MAX_INDEX && res == TRUE; ++i ) { - res = ( fIntervalPatterns[i].firstPart == - fmt->fIntervalPatterns[i].firstPart) && - ( fIntervalPatterns[i].secondPart == - fmt->fIntervalPatterns[i].secondPart ) && - ( fIntervalPatterns[i].laterDateFirst == - fmt->fIntervalPatterns[i].laterDateFirst) ; - } - return res; - } - return FALSE; -} + if (typeid(*this) != typeid(other)) {return FALSE;} + const DateIntervalFormat* fmt = (DateIntervalFormat*)&other; + if (this == fmt) {return TRUE;} + if (!Format::operator==(other)) {return FALSE;} + if ((fInfo != fmt->fInfo) && (fInfo == NULL || fmt->fInfo == NULL)) {return FALSE;} + if (fInfo && fmt->fInfo && (*fInfo != *fmt->fInfo )) {return FALSE;} + { + Mutex lock(&gFormatterMutex); + if (fDateFormat != fmt->fDateFormat && (fDateFormat == NULL || fmt->fDateFormat == NULL)) {return FALSE;} + if (fDateFormat && fmt->fDateFormat && (*fDateFormat != *fmt->fDateFormat)) {return FALSE;} + // TODO: should operator == ignore the From and ToCalendar? They hold transient values during + // formatting of a DateInterval. + if (fFromCalendar != fmt->fFromCalendar && (fFromCalendar == NULL || fmt->fFromCalendar == NULL)) {return FALSE;} + if (fFromCalendar && fmt->fFromCalendar && !fFromCalendar->isEquivalentTo(*fmt->fFromCalendar)) {return FALSE;} + + if (fToCalendar != fmt->fToCalendar && (fToCalendar == NULL || fmt->fToCalendar == NULL)) {return FALSE;} + if (fToCalendar && fmt->fToCalendar && !fToCalendar->isEquivalentTo(*fmt->fToCalendar)) {return FALSE;} + } + if (fSkeleton != fmt->fSkeleton) {return FALSE;} + if (fDatePattern != fmt->fDatePattern && (fDatePattern == NULL || fmt->fDatePattern == NULL)) {return FALSE;} + if (fDatePattern && fmt->fDatePattern && (*fDatePattern != *fmt->fDatePattern)) {return FALSE;} + if (fTimePattern != fmt->fTimePattern && (fTimePattern == NULL || fmt->fTimePattern == NULL)) {return FALSE;} + if (fTimePattern && fmt->fTimePattern && (*fTimePattern != *fmt->fTimePattern)) {return FALSE;} + if (fDateTimeFormat != fmt->fDateTimeFormat && (fDateTimeFormat == NULL || fmt->fDateTimeFormat == NULL)) {return FALSE;} + if (fDateTimeFormat && fmt->fDateTimeFormat && (*fDateTimeFormat != *fmt->fDateTimeFormat)) {return FALSE;} + if (fLocale != fmt->fLocale) {return FALSE;} + + for (int32_t i = 0; i< DateIntervalInfo::kIPI_MAX_INDEX; ++i ) { + if (fIntervalPatterns[i].firstPart != fmt->fIntervalPatterns[i].firstPart) {return FALSE;} + if (fIntervalPatterns[i].secondPart != fmt->fIntervalPatterns[i].secondPart ) {return FALSE;} + if (fIntervalPatterns[i].laterDateFirst != fmt->fIntervalPatterns[i].laterDateFirst) {return FALSE;} + } + return TRUE; +} UnicodeString& @@ -259,7 +256,7 @@ DateIntervalFormat::format(const Formattable& obj, if ( obj.getType() == Formattable::kObject ) { const UObject* formatObj = obj.getObject(); const DateInterval* interval = dynamic_cast(formatObj); - if (interval != NULL){ + if (interval != NULL) { return format(interval, appendTo, fieldPosition, status); } } @@ -276,16 +273,15 @@ DateIntervalFormat::format(const DateInterval* dtInterval, if ( U_FAILURE(status) ) { return appendTo; } - - if ( fFromCalendar != NULL && fToCalendar != NULL && - fDateFormat != NULL && fInfo != NULL ) { - fFromCalendar->setTime(dtInterval->getFromDate(), status); - fToCalendar->setTime(dtInterval->getToDate(), status); - if ( U_SUCCESS(status) ) { - return format(*fFromCalendar, *fToCalendar, appendTo,fieldPosition, status); - } + if (fFromCalendar == NULL || fToCalendar == NULL || fDateFormat == NULL || fInfo == NULL) { + status = U_INVALID_STATE_ERROR; + return appendTo; } - return appendTo; + + Mutex lock(&gFormatterMutex); + fFromCalendar->setTime(dtInterval->getFromDate(), status); + fToCalendar->setTime(dtInterval->getToDate(), status); + return formatImpl(*fFromCalendar, *fToCalendar, appendTo,fieldPosition, status); } @@ -295,6 +291,17 @@ DateIntervalFormat::format(Calendar& fromCalendar, UnicodeString& appendTo, FieldPosition& pos, UErrorCode& status) const { + Mutex lock(&gFormatterMutex); + return formatImpl(fromCalendar, toCalendar, appendTo, pos, status); +} + + +UnicodeString& +DateIntervalFormat::formatImpl(Calendar& fromCalendar, + Calendar& toCalendar, + UnicodeString& appendTo, + FieldPosition& pos, + UErrorCode& status) const { if ( U_FAILURE(status) ) { return appendTo; } @@ -436,7 +443,7 @@ DateIntervalFormat::setDateIntervalInfo(const DateIntervalInfo& newItvPattern, delete fDateTimeFormat; fDateTimeFormat = NULL; - if ( fDateFormat ) { + if (fDateFormat) { initializePattern(status); } } @@ -487,6 +494,7 @@ const TimeZone& DateIntervalFormat::getTimeZone() const { if (fDateFormat != NULL) { + Mutex lock(&gFormatterMutex); return fDateFormat->getTimeZone(); } // If fDateFormat is NULL (unexpected), create default timezone. @@ -506,37 +514,21 @@ DateIntervalFormat::DateIntervalFormat(const Locale& locale, fTimePattern(NULL), fDateTimeFormat(NULL) { - if ( U_FAILURE(status) ) { - delete dtItvInfo; - return; - } - SimpleDateFormat* dtfmt = - static_cast( - DateFormat::createInstanceForSkeleton( - *skeleton, locale, status)); - if ( U_FAILURE(status) ) { - delete dtItvInfo; - delete dtfmt; - return; - } - if ( dtfmt == NULL || dtItvInfo == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - // safe to delete NULL - delete dtfmt; - delete dtItvInfo; + LocalPointer info(dtItvInfo, status); + LocalPointer dtfmt(static_cast( + DateFormat::createInstanceForSkeleton(*skeleton, locale, status)), status); + if (U_FAILURE(status)) { return; } + if ( skeleton ) { fSkeleton = *skeleton; } - fInfo = dtItvInfo; - fDateFormat = dtfmt; - if ( dtfmt->getCalendar() ) { - fFromCalendar = dtfmt->getCalendar()->clone(); - fToCalendar = dtfmt->getCalendar()->clone(); - } else { - fFromCalendar = NULL; - fToCalendar = NULL; + fInfo = info.orphan(); + fDateFormat = dtfmt.orphan(); + if ( fDateFormat->getCalendar() ) { + fFromCalendar = fDateFormat->getCalendar()->clone(); + fToCalendar = fDateFormat->getCalendar()->clone(); } initializePattern(status); } @@ -771,7 +763,7 @@ DateIntervalFormat::initializePattern(UErrorCode& status) { * range expression for the time. */ - if ( fDateTimeFormat == 0 ) { + if ( fDateTimeFormat == NULL ) { // earlier failure getting dateTimeFormat return; } diff --git a/icu4c/source/i18n/unicode/dtitvfmt.h b/icu4c/source/i18n/unicode/dtitvfmt.h index d163bad60f4..1509fa39fdc 100644 --- a/icu4c/source/i18n/unicode/dtitvfmt.h +++ b/icu4c/source/i18n/unicode/dtitvfmt.h @@ -1,5 +1,5 @@ /******************************************************************************** -* Copyright (C) 2008-2013,2015, International Business Machines Corporation and +* Copyright (C) 2008-2016, International Business Machines Corporation and * others. All Rights Reserved. ******************************************************************************* * @@ -504,7 +504,13 @@ public: /** - * Gets the date formatter + * Gets the date formatter. The DateIntervalFormat instance continues to own + * the returned DateFormatter object, and will use and possibly modify it + * during format operations. In a multi-threaded environment, the returned + * DateFormat can only be used if it is certain that no other threads are + * concurrently using this DateIntervalFormatter, even for nominally const + * functions. + * * @return the date formatter associated with this date interval formatter. * @stable ICU 4.0 */ @@ -685,6 +691,8 @@ private: * The full pattern used in this fall-back format is the * full pattern of the date formatter. * + * gFormatterMutex must already be locked when calling this function. + * * @param fromCalendar calendar set to the from date in date interval * to be formatted into date interval string * @param toCalendar calendar set to the to date in date interval @@ -697,6 +705,7 @@ private: * On output: the offsets of the alignment field. * @param status output param set to success/failure code on exit * @return Reference to 'appendTo' parameter. + * @internal */ UnicodeString& fallbackFormat(Calendar& fromCalendar, Calendar& toCalendar, @@ -952,6 +961,37 @@ private: const UnicodeString* secondPart, UBool laterDateFirst); + /** + * Format 2 Calendars to produce a string. + * Implementation of the similar public format function. + * Must be called with gFormatterMutex already locked. + * + * Note: "fromCalendar" and "toCalendar" are not const, + * since calendar is not const in SimpleDateFormat::format(Calendar&), + * + * @param fromCalendar calendar set to the from date in date interval + * to be formatted into date interval string + * @param toCalendar calendar set to the to date in date interval + * to be formatted into date interval string + * @param appendTo Output parameter to receive result. + * Result is appended to existing contents. + * @param fieldPosition On input: an alignment field, if desired. + * On output: the offsets of the alignment field. + * There may be multiple instances of a given field type + * in an interval format; in this case the fieldPosition + * offsets refer to the first instance. + * @param status Output param filled with success/failure status. + * Caller needs to make sure it is SUCCESS + * at the function entrance + * @return Reference to 'appendTo' parameter. + * @internal + */ + UnicodeString& formatImpl(Calendar& fromCalendar, + Calendar& toCalendar, + UnicodeString& appendTo, + FieldPosition& fieldPosition, + UErrorCode& status) const ; + // from calendar field to pattern letter static const UChar fgCalendarFieldToPatternLetter[]; diff --git a/icu4c/source/test/intltest/dtifmtts.cpp b/icu4c/source/test/intltest/dtifmtts.cpp index 02b1f05c79a..16c94d1164f 100644 --- a/icu4c/source/test/intltest/dtifmtts.cpp +++ b/icu4c/source/test/intltest/dtifmtts.cpp @@ -1,7 +1,7 @@ /******************************************************************** * COPYRIGHT: - * Copyright (c) 1997-2015, International Business Machines Corporation and + * Copyright (c) 1997-2016, International Business Machines Corporation and * others. All Rights Reserved. ********************************************************************/ @@ -18,9 +18,11 @@ #include #endif - -#include "cstring.h" #include "dtifmtts.h" + +#include "cstr.h" +#include "cstring.h" +#include "simplethread.h" #include "unicode/gregocal.h" #include "unicode/dtintrv.h" #include "unicode/dtitvinf.h" @@ -51,6 +53,7 @@ void DateIntervalFormatTest::runIndexedTest( int32_t index, UBool exec, const ch TESTCASE(5, testStress); TESTCASE(6, testTicket11583_2); TESTCASE(7, testTicket11985); + TESTCASE(8, testTicket11669); default: name = ""; break; } } @@ -128,7 +131,7 @@ void DateIntervalFormatTest::testAPI() { DateIntervalFormat* another = (DateIntervalFormat*)dtitvfmt->clone(); if ( (*another) != (*dtitvfmt) ) { - dataerrln("ERROR: clone failed"); + dataerrln("%s:%d ERROR: clone failed", __FILE__, __LINE__); } @@ -1544,4 +1547,65 @@ void DateIntervalFormatTest::testTicket11985() { assertEquals("Format pattern", "h:mm a", pattern); } +// Ticket 11669 - thread safety of DateIntervalFormat::format(). This test failed before +// the implementation was fixed. + +static const DateIntervalFormat *gIntervalFormatter = NULL; // The Formatter to be used concurrently by test threads. +static const DateInterval *gInterval = NULL; // The date interval to be formatted concurrently. +static const UnicodeString *gExpectedResult = NULL; + +void DateIntervalFormatTest::threadFunc11669(int32_t threadNum) { + (void)threadNum; + for (int loop=0; loop<1000; ++loop) { + UErrorCode status = U_ZERO_ERROR; + FieldPosition pos(0); + UnicodeString result; + gIntervalFormatter->format(gInterval, result, pos, status); + if (U_FAILURE(status)) { + errln("%s:%d %s", __FILE__, __LINE__, u_errorName(status)); + return; + } + if (result != *gExpectedResult) { + errln("%s:%d Expected \"%s\", got \"%s\"", __FILE__, __LINE__, CStr(*gExpectedResult)(), CStr(result)()); + return; + } + } +} + +void DateIntervalFormatTest::testTicket11669() { + UErrorCode status = U_ZERO_ERROR; + LocalPointer formatter(DateIntervalFormat::createInstance(UDAT_YEAR_MONTH_DAY, Locale::getEnglish(), status), status); + LocalPointer tz(TimeZone::createTimeZone("America/Los_Angleles"), status); + LocalPointer intervalStart(Calendar::createInstance(*tz, Locale::getEnglish(), status), status); + LocalPointer intervalEnd(Calendar::createInstance(*tz, Locale::getEnglish(), status), status); + if (U_FAILURE(status)) { + errln("%s:%d %s", __FILE__, __LINE__, u_errorName(status)); + return; + } + + intervalStart->set(2009, 6, 1, 14, 0); + intervalEnd->set(2009, 6, 2, 14, 0); + DateInterval interval(intervalStart->getTime(status), intervalEnd->getTime(status)); + FieldPosition pos(0); + UnicodeString expectedResult; + formatter->format(&interval, expectedResult, pos, status); + if (U_FAILURE(status)) { + errln("%s:%d %s", __FILE__, __LINE__, u_errorName(status)); + return; + } + + gInterval = &interval; + gIntervalFormatter = formatter.getAlias(); + gExpectedResult = &expectedResult; + + ThreadPool threads(this, 4, &DateIntervalFormatTest::threadFunc11669); + threads.start(); + threads.join(); + + gInterval = NULL; // Don't leave dangling pointers lying around. Not strictly necessary. + gIntervalFormatter = NULL; + gExpectedResult = NULL; +} + + #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/test/intltest/dtifmtts.h b/icu4c/source/test/intltest/dtifmtts.h index af4318c5fd8..5fb64f2b2ef 100644 --- a/icu4c/source/test/intltest/dtifmtts.h +++ b/icu4c/source/test/intltest/dtifmtts.h @@ -1,6 +1,6 @@ /******************************************************************** * COPYRIGHT: - * Copyright (c) 2008-2015 International Business Machines Corporation and + * Copyright (c) 2008-2016 International Business Machines Corporation and * others. All Rights Reserved. ********************************************************************/ @@ -56,6 +56,9 @@ public: void testTicket11985(); + void testTicket11669(); + void threadFunc11669(int32_t threadNum); + private: /** * Test formatting against expected result