Game Development Community

File::open() not thread safe

by Tom Spilman · in Torque Game Engine · 08/28/2007 (12:35 pm) · 8 replies

The File::open() call in platformWin32\winFileio.cc is not thread safe and can on occasion cause the wrong file to be opened or a failure to open a file when using the File or FileStream from a secondary thread.

The problem is obvious and typical in a non-thread safe engine...

File::Status File::open(const char *filename, const AccessMode openMode)
{
   static char filebuf[2048];
   dStrcpy(filebuf, filename);
   backslash(filebuf);
#ifdef UNICODE
   UTF16 fname[2048];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, sizeof(fname));
#else
   char *fname;
   fname = filebuf;
#endif

... those statics can be corrupted by a thread which then causes havoc with other file opens going on in other threads. The fix is to use the ever so handy FrameTemp<> template:

Edit: I pulled the fix as now realize... i think... that FrameTemp<> and the FrameAllocator are not thread safe themselves. Still investigating...
Tom Spilman's profile picture
Associate Tom Spilman
Sickhead Games

About the author

Tom is a programmer and co-owner of Sickhead Games, LLC.

Recent Threads

  • Adding FXAA to T3D - Live Blog!
  • Sickhead Hiring Artist and Programmer
  • Networking Physics
    • NextNext Thread
    PrevPrevious Thread
    More from Torque Game Engine


    Torque Owner William Todd Scott
    #1
    08/28/2007 (12:52 pm)
    Hi Tom,

    We are starting to look at threading more of the engine, so these posts are very interesting/helpful.

    Todd

    Tim McClarren
    Default Studio Name
    #2
    08/28/2007 (1:01 pm)
    FrameAllocator is definitely not thread-safe.

    What about using a thread-local here?

    It seems like introducing thread-locals for all function and block-scoped statics (and class statics too) would help immensely. There seems to be quite a few of them.

    Associate Tom Spilman
    Sickhead Games
    #3
    08/28/2007 (4:33 pm)
    @Tim - Well i guess since this is Win32 specific thread-locals might be a solution here, but a non-platform specific solution would be best.

    Realistically... the optimization here is silly . Its not like you call File::open() thousands of times per rendered frame or that a stack allocation of 2048 bytes is slow... on the contrary stack allocation of a primitive array is stupid fast. So its no performance gain to do any of it this way... its premature optimization at its finest.

    So short of a thread safe FrameAllocator (which we need too anyway) the right fix is....

    File::Status File::open(const char *filename, const AccessMode openMode)
{
   char filebuf[MAX_PATH];
   dStrcpy(filebuf, filename);
   backslash(filebuf);
#ifdef UNICODE
   UTF16 fname[MAX_PATH];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, sizeof(fname));
#else
   char *fname;
   fname = filebuf;
#endif

    ... the only change here being the use of MAX_PATH which on windows is 260 characters and should be safe to assume (although i don't know about XBox) and making filebuf non-static.

    Associate Orion Elenzil
    Real Life Plus
    #4
    08/28/2007 (5:48 pm)
    Heh, good point Tom.

    prolly want to use dStrncpy(filebuf, filename, sizeof(filebuf)); tho.

    Associate Tom Spilman
    Sickhead Games
    #5
    08/28/2007 (6:24 pm)
    @Orion - Good catch! If i'm gonna fix it i might as well fix it right... but there is a minor bug in that as well. If you do get a filename longer than MAX_PATH then you would copy it but omit the trailing /0... not good!

    char filebuf[MAX_PATH];
   dStrncpy(filebuf, filename, MAX_PATH);
   filebuf[MAX_PATH-1] = 0;

    Not as pretty... but it is safe.

    Second question.... is this right?

    UTF16 fname[MAX_PATH];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, sizeof(fname));

    ... sizeof(fname) returns the size of fname in bytes... not the number of UTF16 in the array. So is this really what convertUTF8toUTF16() expects... the number of bytes in the array or the number of UTF16s?

    Torque Owner Marcel Meyer
    Kornner Studios
    #6
    09/23/2007 (6:50 am)
    Looking at the dPathCopy function:

    #ifdef UNICODE
   UTF16 f[1024], t[1024];
   convertUTF8toUTF16((UTF8 *)fromName, f, sizeof(f));
   convertUTF8toUTF16((UTF8 *)toName, t, sizeof(t));
#else
   char *f = (char*)fromName;
   char *t = (char*)toName;
#endif
    So I'm pretty sure its the acual size of the array in bytes, not elements

    -kornman00

    Associate Tom Spilman
    Sickhead Games
    #7
    10/06/2007 (2:52 pm)
    It's been pretty much confirmed that this is a bug. All cases of ...

    UTF16 fname[MAX_PATH];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, sizeof(fname));

    ... should be changed to ...

    UTF16 fname[MAX_PATH];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, MAX_PATH);

    ... this is a bug in alot of areas in the engine. Still its one that will rarely if ever hit... you almost never get a MAX_PATH file name.

    Torque Owner Michael Bacon
    Default Studio Name
    #8
    10/13/2007 (6:46 pm)
    An alternate fix would be:

    UTF16 fname[MAX_PATH];
   convertUTF8toUTF16((UTF8 *)filebuf, fname, sizeof(fname) [b]/ sizeof(fname[0])[/b]);

    which works in all cases...