Opened 6 years ago

Closed 6 years ago

#3979 closed defect (fixed)

Avoid runtime locale switching (CPLLocaleC usage)

Reported by: dron Owned by: warmerdam
Priority: normal Milestone: 1.9.0
Component: GDAL_Raster Version: unspecified
Severity: blocker Keywords: locale, atof
Cc:

Description

Frank,

r21801 as well as r21785 and r21784 introduces multiple runtime locale switches in GDAL. That is generally bad idea, because POSIX locale is a process property, so it is not thread safe and performance overhead is also remains unclear. Note, that if the main process opens several GDAL datasets in separate threads setlocale() call changes behavior of the whole process including the possible separate thread that interacts with user.

There is a CPLAtof() helper that should solve the problem of parsing numeric values. Is it possible to go with CPLAtof() not touching locale at all?

The second problem is that GDAL now can't be built with an external libgeotiff because of r21801, the CPLLocaleC is unavailable. It's declaration should be moved into the gt_wkt_srs.cpp in the block, surrounded by CPL_SERV_H_INTERNAL guards.

Best regards,
Andrey

Attachments (1)

gt_wkt_srs.diff (2.6 KB) - added by dron 6 years ago.
Replace atof() calls with CPLAtof() to avoid locale dependend problems

Download all attachments as: .zip

Change History (4)

Changed 6 years ago by dron

Replace atof() calls with CPLAtof() to avoid locale dependend problems

comment:1 Changed 6 years ago by warmerdam

  • Status changed from new to assigned

I have reverted the gt_wkt_srs.cpp change which broke the build (r21829). Sorry about that.

I agree where possible use of CPLAtof() is preferred to use of CPLLocaleC; however, CPLLocaleC is in use in various locations. Removal of this will require some further discussion as it also affects formatting of results produced.

I don't think the provided diff corrects for all places the locale was an issue. The change in gt_wkt_srs.cpp was mostly intended to alter the behavior of plain atof() calls down in libgeotiff.

I have moved usgsdemdataset.cpp to use CPLAtof() instead of CPLLocaleC (r21830,r21831).

comment:2 Changed 6 years ago by warmerdam

I have slightly modified the CPLLocaleC class (r21835) so the constructor is:

CPLLocaleC::CPLLocaleC() : pszOldLocale(CPLStrdup(setlocale(LC_NUMERIC,NULL)))

{
    if( CSLTestBoolean(CPLGetConfigOption("GDAL_DISABLE_CPLLOCALEC","NO"))
        || EQUAL(pszOldLocale,"C")
        || EQUAL(pszOldLocale,"POSIX")
        || setlocale(LC_NUMERIC,"C") == NULL )
    {
        CPLFree( pszOldLocale );
        pszOldLocale = NULL;
    }
}

This should avoid calling setlocale() to alter the locale if it is already "C" or "POSIX". This is to partially address the performance concern. The check of the CPLConfigOption is also new, and allows an application to disable our setting of locales though at the risk of some breakage in program behavior. This mechanism also makes it easier to reproduce locale problems when trying to isolate needed changes.

comment:3 Changed 6 years ago by warmerdam

  • Milestone set to 1.9.0
  • Resolution set to fixed
  • Status changed from assigned to closed

I have updated libgeotiff to have it's own locale safe atof() derived from GDAL's and use it in geo_normalize.c.

http://trac.osgeo.org/geotiff/changeset/1979

I have updated GDAL to utilize GTIFAtof() and cleared away use of CPLLocaleC for the GeoTIFF code in trunk (r21839, r21838).

I am closing this ticket as the new problem is resolved.

If we want to totally eradicate use of CPLLocaleC in the GDAL library that will need to be driven by discussion on gdal-dev and/or an RFC.

Note: See TracTickets for help on using tickets.