Opened 7 years ago

Closed 7 years ago

#6223 closed defect (fixed)

CPLGenerateTempFilename volatile and/or CPLAtomicInc

Reported by: Kurt Schwehr Owned by: Kurt Schwehr
Priority: normal Milestone:
Component: default Version: svn-trunk
Severity: normal Keywords: multithreading
Cc:

Description (last modified by Even Rouault)

In r15574, you put a volatile modifier on nTempFileCounter. This looks to be protect from multithreaded race cases between callers of CPLGenerateTempFilename where more than one thread might get the same tempfile number. Would it be better to call CPLAtomicInc and use the result of that? If so, should I drop the volatile?

e.g.

    static volatile int nTempFileCounter = 0;
    CPLString osFilename;
    osFilename.Printf( "%s%u_%d", pszStem,
                       (unsigned int)(CPLGetPID() & 0xFFFFFFFFU), nTempFileCounter++ );

Would become this.

    static int nTempFileCounter = 0;
    CPLString osFilename;
    osFilename.Printf( "%s%u_%d", pszStem,
                       static_cast<unsigned int>( CPLGetPID() & 0xFFFFFFFFU),
                       CPLAtomicInc(nTempFileCounter) );

A second question on this code:

Why the mask on the CPLGetPID()?

Change History (7)

comment:1 by Kurt Schwehr, 7 years ago

Description: modified (diff)
Status: newassigned

comment:2 by Even Rouault, 7 years ago

Description: modified (diff)

+1 for CPLAtomicInc (but use &nTempFileCounter as argument)

I think the mask was when using with some sanitzation runtime because CPLGetPID() could return a value larger than a unsigned int. But A better fix would be to replace %u with CPL_FRMT_GIB and dropping the cast

comment:3 by Kurt Schwehr, 7 years ago

Bugger... forgot to include this ticket in the commit message of r31513 on trunk.

comment:4 by Kurt Schwehr, 7 years ago

Submitted r31516 to make a TODO that all this can be done in C++11 with std::atomic.

comment:5 by Kurt Schwehr, 7 years ago

Summary: CPLGenerateTempFilename volatite and/or CPLAtomicIncCPLGenerateTempFilename volatile and/or CPLAtomicInc

comment:6 by Even Rouault, 7 years ago

I assume this can be closed now ?

comment:7 by Kurt Schwehr, 7 years ago

Resolution: fixed
Status: assignedclosed

Closing. Resolved with r31513.

Note: See TracTickets for help on using tickets.