We're updating the issue view to help you get more done. 

C++: clean up object adoption semantics & implementation in failure cases

Description

This was triggered by a test that failed with an access violation on Windows.

In the ICU C++ code, many constructors and factory functions (createInstance())
adopt objects. To adopt means that they get passed in a pointer to such an
adoptee object and then take over ownership and lifecycle control.

Right now, the semantics is specified such that the adoption is only completed
on successful creation of the new object.
However, the implementation makes it hard to determine if the adoption
actually did take place.

For example, consider the following Calendar::createInstance() as
it is used by ucal_open().
If the UErrorCode is initially U_SUCCESS() and the memory allocation
succeeds but the GregorianCalendar constructor fails, then
createInstance() deletes the "bad" object.
The GregorianCalendar had been set up far enough to adopt the TimeZone
object, which is now deleted with the GregorianCalendar object.
ucal_open() detects the 0 pointer return value but does not check for
the UErrorCode and assumes that the memory allocation had failed.
It attempts to delete the TimeZone object, which is an error and
resulted in an access violation in a test.

We are going to change the semantics to be more consistent and robust:

The adoption will be specified to complete as soon as the constructor
or factory function is called, whether it succeeds or not.
In a failure case a factory function still deletes a "bad" object
and returns a 0 pointer.
It will now also set a U_MEMORY_ALLOCATION_ERROR itself if it is the
allocation that failed - to keep the caller from second-guessing.

This new semantics should make it easier to directly cascade
function calls that take UErrorCode parameters without having
to check for the success of each function call.

Below see first the current code pieces in the case of ucal_open()
and then the fixed code piece.
I will make this change in the coming days.

markus

current, problematic implementation ---------------------------------
calendar.h ----------------------------------------------------------
/**

  • Creates a Calendar using the given timezone and given locale. If creation
    of

  • a new Calendar is successful, the Calendar takes ownership of
    zoneToAdopt; the

  • client should not delete it.
    *

  • @param zoneToAdopt The given timezone to be adopted.

  • @param aLocale The given locale.

  • @param success Indicates the success/failure of Calendar creation.
    Filled in

  • with U_ZERO_ERROR if created successfully, set to a
    failure result

  • otherwise.

  • @return A Calendar if created successfully. NULL otherwise.
    */
    static Calendar* createInstance(TimeZone* zoneToAdopt, const Locale&
    aLocale, UErrorCode& success);

calendar.cpp --------------------------------------------------------
Calendar*
Calendar::createInstance(TimeZone* zone, UErrorCode& success)
{
if (FAILURE(success)) return 0;
// since the Locale isn't specified, use the default locale
Calendar* c = new GregorianCalendar(zone, Locale::getDefault(), success);
if (FAILURE(success)) { delete c; c = 0; }
return c;
}

ucal.cpp ------------------------------------------------------------
CAPI UCalendar*
ucal_open(const UChar* zoneID,
int32_t len,
const char* locale,
UCalendarType type,
UErrorCode* status)
{
if(FAILURE(*status)) return 0;

TimeZone *zone = 0;
if(zoneID == 0) {
zone = TimeZone::createDefault();
}
else {
int32_t length = (len == -1 ? u_strlen(zoneID) : len);

zone = TimeZone::createTimeZone(UnicodeString((UChar*)zoneID,
length, length));
}
if(zone == 0) {
*status = U_MEMORY_ALLOCATION_ERROR;
return 0;
}

Calendar *cal = 0;
cal = Calendar::createInstance(zone, Locale().init(locale), *status);
if(cal == 0) {
*status = U_MEMORY_ALLOCATION_ERROR;
delete zone;
return 0;
}

return (UCalendar*)cal;
}

fixed code ----------------------------------------------------------
calendar.h ----------------------------------------------------------
/**

  • Creates a Calendar using the given timezone and given locale.

  • The Calendar takes ownership of zoneToAdopt; the

  • client must not delete it.
    *

  • @param zoneToAdopt The given timezone to be adopted.

  • @param aLocale The given locale.

  • @param success Indicates the success/failure of Calendar creation.
    Filled in

  • with U_ZERO_ERROR if created successfully, set to a
    failure result

  • otherwise.

  • @return A Calendar if created successfully. NULL otherwise.
    */
    static Calendar* createInstance(TimeZone* zoneToAdopt, const Locale&
    aLocale, UErrorCode& success);

calendar.cpp --------------------------------------------------------
Calendar*
Calendar::createInstance(TimeZone* zone, UErrorCode& errorCode) {
if(FAILURE(errorCode)) {
delete zone;
return 0;
}
// since the Locale isn't specified, use the default locale
Calendar* c = new GregorianCalendar(zone, Locale::getDefault(), errorCode);
if(c == 0) {
errorCode = U_MEMORY_ALLOCATION_ERROR;
delete zone;
} else if(FAILURE(errorCode)) {
delete c;
c = 0;
}
return c;
}

ucal.cpp ------------------------------------------------------------
CAPI UCalendar*
ucal_open(const UChar* zoneID,
int32_t len,
const char* locale,
UCalendarType type,
UErrorCode* status)
{
if(FAILURE(*status)) return 0;

TimeZone *zone = 0;
if(zoneID == 0) {
zone = TimeZone::createDefault();
}
else {
int32_t length = (len == -1 ? u_strlen(zoneID) : len);

zone = TimeZone::createTimeZone(UnicodeString((UChar*)zoneID,
length, length));
}
if(zone == 0) {
*status = U_MEMORY_ALLOCATION_ERROR;
return 0;
}

return (UCalendar*)Calendar::createInstance(zone, Locale().init(locale),
*status);
}

Status

Assignee

TracBot

Reporter

TracBot

Labels

Reviewer

None

Time Needed

None

Start date

None

Components

Priority

assess