From e5fac0714c86885e32b18b7255b7217fa3382cfa Mon Sep 17 00:00:00 2001 From: "Steven R. Loomis" Date: Thu, 5 Sep 2013 05:32:56 +0000 Subject: [PATCH] ICU-10345 make formatting of a Formattable currency thread safe.. by cloning. makes unum_formatDoubleCurrency also slow. X-SVN-Rev: 34200 --- icu4c/source/i18n/numfmt.cpp | 65 ++++++++---- icu4c/source/test/cintltst/cnumtst.c | 15 +-- icu4c/source/test/intltest/tsmthred.cpp | 126 +++++++++++++++++++++++- 3 files changed, 177 insertions(+), 29 deletions(-) diff --git a/icu4c/source/i18n/numfmt.cpp b/icu4c/source/i18n/numfmt.cpp index 35fc7723faa..0e2aa64b2e3 100644 --- a/icu4c/source/i18n/numfmt.cpp +++ b/icu4c/source/i18n/numfmt.cpp @@ -433,24 +433,30 @@ NumberFormat::format(const StringPiece &decimalNum, return toAppendTo; } -// ------------------------------------- +/** + * // Formats the number object and save the format // result in the toAppendTo string buffer. // utility to save/restore state, used in two overloads // of format(const Formattable&...) below. - +* +* Old purpose of ArgExtractor was to avoid const. Not thread safe! +* +* keeping it around as a shim. +*/ class ArgExtractor { - NumberFormat *ncnf; const Formattable* num; - UBool setCurr; UChar save[4]; + UBool fWasCurrency; public: ArgExtractor(const NumberFormat& nf, const Formattable& obj, UErrorCode& status); ~ArgExtractor(); const Formattable* number(void) const; + const UChar *iso(void) const; + const UBool wasCurrency(void) const; }; inline const Formattable* @@ -458,33 +464,34 @@ ArgExtractor::number(void) const { return num; } -// TODO: Thread safety problem? the function of ArgExtractor appears to be to temporarily -// setCurrency() on the number formatter, then restore it after the format() is complete. -// Another thread could be using the formatter. +inline const UBool +ArgExtractor::wasCurrency(void) const { + return fWasCurrency; +} + +inline const UChar * +ArgExtractor::iso(void) const { + return save; +} -ArgExtractor::ArgExtractor(const NumberFormat& nf, const Formattable& obj, UErrorCode& status) - : ncnf((NumberFormat*) &nf), num(&obj), setCurr(FALSE) { +ArgExtractor::ArgExtractor(const NumberFormat& nf, const Formattable& obj, UErrorCode& /*status*/) + : num(&obj), fWasCurrency(FALSE) { const UObject* o = obj.getObject(); // most commonly o==NULL const CurrencyAmount* amt; if (o != NULL && (amt = dynamic_cast(o)) != NULL) { // getISOCurrency() returns a pointer to internal storage, so we // copy it to retain it across the call to setCurrency(). - const UChar* curr = amt->getISOCurrency(); - u_strcpy(save, nf.getCurrency()); - setCurr = (u_strcmp(curr, save) != 0); - if (setCurr) { - ncnf->setCurrency(curr, status); - } + //const UChar* curr = amt->getISOCurrency(); + u_strcpy(save, amt->getISOCurrency()); num = &amt->getNumber(); + fWasCurrency=TRUE; + } else { + save[0]=0; } } ArgExtractor::~ArgExtractor() { - if (setCurr) { - UErrorCode ok = U_ZERO_ERROR; - ncnf->setCurrency(save, ok); // always restore currency - } } UnicodeString& NumberFormat::format(const DigitList &number, @@ -530,6 +537,16 @@ NumberFormat::format(const Formattable& obj, ArgExtractor arg(*this, obj, status); const Formattable *n = arg.number(); + const UChar *iso = arg.iso(); + + if(arg.wasCurrency() && u_strcmp(iso, getCurrency())) { + // trying to format a different currency. + // Right now, we clone. + LocalPointer cloneFmt((NumberFormat*)this->clone()); + cloneFmt->setCurrency(iso, status); + // next line should NOT recurse, because n is numeric whereas obj was a wrapper around currency amount. + return cloneFmt->format(*n, appendTo, pos, status); + } if (n->isNumeric() && n->getDigitList() != NULL) { // Decimal Number. We will have a DigitList available if the value was @@ -575,6 +592,16 @@ NumberFormat::format(const Formattable& obj, ArgExtractor arg(*this, obj, status); const Formattable *n = arg.number(); + const UChar *iso = arg.iso(); + + if(arg.wasCurrency() && u_strcmp(iso, getCurrency())) { + // trying to format a different currency. + // Right now, we clone. + LocalPointer cloneFmt((NumberFormat*)this->clone()); + cloneFmt->setCurrency(iso, status); + // next line should NOT recurse, because n is numeric whereas obj was a wrapper around currency amount. + return cloneFmt->format(*n, appendTo, posIter, status); + } if (n->isNumeric() && n->getDigitList() != NULL) { // Decimal Number diff --git a/icu4c/source/test/cintltst/cnumtst.c b/icu4c/source/test/cintltst/cnumtst.c index 1398dc97632..8e6cfda084e 100644 --- a/icu4c/source/test/cintltst/cnumtst.c +++ b/icu4c/source/test/cintltst/cnumtst.c @@ -290,8 +290,10 @@ static void TestNumberFormat() } if(result && u_strcmp(result, temp1)==0) log_verbose("Pass: Number Formatting using unum_formatDouble() Successful\n"); - else - log_err("FAIL: Error in number formatting using unum_formatDouble()\n"); + else { + log_err("FAIL: Error in number formatting using unum_formatDouble() - got '%s' expected '%s'\n", + aescstrdup(result, -1), aescstrdup(temp1, -1)); + } if(pos2.beginIndex == 9 && pos2.endIndex == 11) log_verbose("Pass: Complete number formatting using unum_format() successful\n"); else @@ -336,12 +338,13 @@ static void TestNumberFormat() unum_formatDoubleCurrency(cur_def, a, temp, result, resultlength, &pos2, &status); } if (U_FAILURE(status)) { - log_err("Error in formatting using unum_formatDouble(.....): %s\n", myErrorName(status)); + log_err("Error in formatting using unum_formatDoubleCurrency(.....): %s\n", myErrorName(status)); } if (result && u_strcmp(result, temp1)==0) { - log_verbose("Pass: Number Formatting using unum_formatDouble() Successful\n"); + log_verbose("Pass: Number Formatting using unum_formatDoubleCurrency() Successful\n"); } else { - log_err("FAIL: Error in number formatting using unum_formatDouble()\n"); + log_err("FAIL: Error in number formatting using unum_formatDoubleCurrency() - got '%s' expected '%s'\n", + aescstrdup(result, -1), aescstrdup(temp1, -1)); } if (pos2.beginIndex == 1 && pos2.endIndex == 6) { log_verbose("Pass: Complete number formatting using unum_format() successful\n"); @@ -364,7 +367,7 @@ static void TestNumberFormat() if (d1!=a1) { log_err("Fail: Error in parsing currency, got %f, expected %f\n", d1, a1); } else { - log_verbose("Pass: parsed currency ammount successfully\n"); + log_verbose("Pass: parsed currency amount successfully\n"); } if (u_strcmp(temp2, temp)==0) { log_verbose("Pass: parsed correct currency\n"); diff --git a/icu4c/source/test/intltest/tsmthred.cpp b/icu4c/source/test/intltest/tsmthred.cpp index af221bab411..f469cfd6662 100644 --- a/icu4c/source/test/intltest/tsmthred.cpp +++ b/icu4c/source/test/intltest/tsmthred.cpp @@ -627,7 +627,7 @@ UnicodeString showDifference(const UnicodeString& expected, const UnicodeString& // //------------------------------------------------------------------------------------------- -const int kFormatThreadIterations = 20; // # of iterations per thread +const int kFormatThreadIterations = 100; // # of iterations per thread const int kFormatThreadThreads = 10; // # of threads to spawn const int kFormatThreadPatience = 60; // time in seconds to wait for all threads @@ -645,7 +645,7 @@ struct FormatThreadTestData // "Someone from {2} is receiving a #{0} error - {1}. Their telephone call is costing {3 number,currency}." -void formatErrorMessage(UErrorCode &realStatus, const UnicodeString& pattern, const Locale& theLocale, +static void formatErrorMessage(UErrorCode &realStatus, const UnicodeString& pattern, const Locale& theLocale, UErrorCode inStatus0, /* statusString 1 */ const Locale &inCountry2, double currency3, // these numbers are the message arguments. UnicodeString &result) { @@ -679,6 +679,102 @@ void formatErrorMessage(UErrorCode &realStatus, const UnicodeString& pattern, co delete fmt; } +/** + * Class for thread-safe (theoretically) format. + * + * + * Its constructor, destructor, and init/fini are NOT thread safe. + */ +class ThreadSafeFormat { +public: + /* give a unique offset to each thread */ + ThreadSafeFormat(); + UBool doStuff(int32_t offset, UnicodeString &appendErr, UErrorCode &status); +private: + LocalPointer fFormat; // formtter - default constructed currency + Formattable fYDDThing; // Formattable currency - YDD + Formattable fBBDThing; // Formattable currency - BBD + + // statics +private: + static LocalPointer gFormat; + static NumberFormat *createFormat(UErrorCode &status); + static Formattable gYDDThing, gBBDThing; +public: + static void init(UErrorCode &status); // avoid static init. + static void fini(UErrorCode &status); // avoid static fini +}; + +LocalPointer ThreadSafeFormat::gFormat; +Formattable ThreadSafeFormat::gYDDThing; +Formattable ThreadSafeFormat::gBBDThing; +UnicodeString gYDDStr, gBBDStr; +NumberFormat *ThreadSafeFormat::createFormat(UErrorCode &status) { + LocalPointer fmt(NumberFormat::createCurrencyInstance(Locale::getUS(), status)); + return fmt.orphan(); +} + + +static const UChar kYDD[] = { 0x59, 0x44, 0x44, 0x00 }; +static const UChar kBBD[] = { 0x42, 0x42, 0x44, 0x00 }; +static const UChar kUSD[] = { 0x55, 0x53, 0x44, 0x00 }; + +void ThreadSafeFormat::init(UErrorCode &status) { + gFormat.adoptInstead(createFormat(status)); + gYDDThing.adoptObject(new CurrencyAmount(123.456, kYDD, status)); + gBBDThing.adoptObject(new CurrencyAmount(987.654, kBBD, status)); + gFormat->format(gYDDThing, gYDDStr, NULL, status); + gFormat->format(gBBDThing, gBBDStr, NULL, status); +} + +void ThreadSafeFormat::fini(UErrorCode &status) { + gFormat.orphan(); +} + +ThreadSafeFormat::ThreadSafeFormat() { +} + +UBool ThreadSafeFormat::doStuff(int32_t offset, UnicodeString &appendErr, UErrorCode &status) { + UBool okay = TRUE; + if(fFormat.isNull()) { + fFormat.adoptInstead(createFormat(status)); + } + + if(u_strcmp(fFormat->getCurrency(), kUSD)) { + appendErr.append("fFormat currency != ") + .append(kUSD) + .append(", =") + .append(fFormat->getCurrency()) + .append("! "); + okay = FALSE; + } + + if(u_strcmp(gFormat->getCurrency(), kUSD)) { + appendErr.append("gFormat currency != ") + .append(kUSD) + .append(", =") + .append(gFormat->getCurrency()) + .append("! "); + okay = FALSE; + } + UnicodeString str; + const UnicodeString *o=NULL; + Formattable f; + const NumberFormat *nf = NULL; // only operate on it as const. + switch(offset%4) { + case 0: f = gYDDThing; o = &gYDDStr; nf = gFormat.getAlias(); break; + case 1: f = gBBDThing; o = &gBBDStr; nf = gFormat.getAlias(); break; + case 2: f = gYDDThing; o = &gYDDStr; nf = fFormat.getAlias(); break; + case 3: f = gBBDThing; o = &gBBDStr; nf = fFormat.getAlias(); break; + } + nf->format(f, str, NULL, status); + + if(*o != str) { + appendErr.append(showDifference(*o, str)); + okay = FALSE; + } + return okay; +} UBool U_CALLCONV isAcceptable(void *, const char *, const char *, const UDataInfo *) { return TRUE; @@ -694,6 +790,8 @@ public: int fNum; int fTraceInfo; + ThreadSafeFormat fTSF; + FormatThreadTest() // constructor is NOT multithread safe. : ThreadWithStatus(), fNum(0), @@ -894,8 +992,16 @@ public: error("PatternFormat: \n" + showDifference(expected,result)); goto cleanupAndReturn; } + // test the Thread Safe Format + UnicodeString appendErr; + if(!fTSF.doStuff(fNum, appendErr, status)) { + error(appendErr); + goto cleanupAndReturn; + } } /* end of for loop */ - + + + cleanupAndReturn: // while (fNum == 4) {SimpleThread::sleep(10000);} // Force a failure by preventing thread from finishing fTraceInfo = 2; @@ -914,6 +1020,11 @@ void MultithreadTest::TestThreadedIntl() UBool haveDisplayedInfo[kFormatThreadThreads]; static const int32_t PATIENCE_SECONDS = 45; + UErrorCode threadSafeErr = U_ZERO_ERROR; + + ThreadSafeFormat::init(threadSafeErr); + assertSuccess("initializing ThreadSafeFormat", threadSafeErr); + // // Create and start the test threads // @@ -936,13 +1047,18 @@ void MultithreadTest::TestThreadedIntl() UBool stillRunning; UDate startTime, endTime; startTime = Calendar::getNow(); + double lastComplaint = 0; do { /* Spin until the test threads complete. */ stillRunning = FALSE; endTime = Calendar::getNow(); - if (((int32_t)(endTime - startTime)/U_MILLIS_PER_SECOND) > PATIENCE_SECONDS) { + double elapsedSeconds = ((int32_t)(endTime - startTime)/U_MILLIS_PER_SECOND); + if (elapsedSeconds > PATIENCE_SECONDS) { errln("Patience exceeded. Test is taking too long."); return; + } else if((elapsedSeconds-lastComplaint) > 2.0) { + infoln("%.1f seconds elapsed (still waiting..)", elapsedSeconds); + lastComplaint = elapsedSeconds; } /* The following sleep must be here because the *BSD operating systems @@ -967,6 +1083,8 @@ void MultithreadTest::TestThreadedIntl() // // All threads have finished. // + ThreadSafeFormat::fini(threadSafeErr); + assertSuccess("finalizing ThreadSafeFormat", threadSafeErr); } #endif /* #if !UCONFIG_NO_FORMATTING */ -- 2.40.0