ubarsc / kealib

KEALib provides an implementation of the GDAL data model. The format supports raster attribute tables, image pyramids, meta-data and in-built statistics while also handling very large files and compression throughout.
http://kealib.org/
MIT License
12 stars 7 forks source link

Add thread safety and quieten hdf5 stack trace #51

Closed gillins closed 8 months ago

gillins commented 9 months ago

This PR is an attempt to solve 2 problems:

  1. kealib is not thread safe
  2. when calling kealib on other threads than the one you used to open/create the file causes hdf5 to be very verbose

More detail below.

kealib is not thread safe

Firstly, the C++ hdf5 bindings aren't thread safe. Secondly, kealib has internal class members that are accessed/modified without appropriate locking. The GDAL driver has had some work to ensure its internal members are protected, but this hasn't been pushed down in to kealib. Doing so will guard against people who are linking to kealib directly (like the Python bindings) and also from GDAL but from a method that has no locking (because there are no internal members used).

This is addressed using std::recursive_mutex everywhere. I've gone for this because it is safest, especially since most things call into the hdf5 C++ bindings. There was a problem with the RAT classes needing to see the same mutex as KEAImageIO (using the C++ bindings) so I've used std::shared_ptr to allow them to share a mutex (and a new base class for both to hold this).

hdf5 error trace

kealib calls Exception::dontPrint on file creation and open. Exception::dontPrint only applies per-thread though. So if you call back into kealib from another thread you may well get a stack trace when it is looking up something that isn't in the file (ie NODATA).

This is addressed by the RAII style KEAStackPrintState class everywhere hdf5 is called that saves the trace printing state and restores it at function end. This should reduce the likelihood of conflict with another library that is calling hdf5.

This will break ABI compatibility with 1.5.x

@petebunting any thoughts?

cc: @neilflood, @petescarth

petebunting commented 9 months ago

Hi @gillins

Sounds good to me; I am not familiar with C++ mutex, so I probably can't comment on it in too much detail, but it is a good development.

gillins commented 9 months ago

Thanks Pete. I forgot to attach the following which demonstrates the problem (and works fine with this PR):

#include <stdio.h>
#include <stdlib.h>
#include <thread>
#include "libkea/KEAImageIO.h"

const int TILESIZE = 100;
const int NUM_THREADS = 40;

void process(kealib::KEAImageIO *io, int tid)
{
    char *pData = (char*)malloc(TILESIZE * TILESIZE * sizeof(char));
    char nodata;
    for( int i = 0; i < 10000; i++ )
    {
       uint64_t x = rand() % 900;
       uint64_t y = rand() % 900;
       fprintf(stderr, "reading %d %d %d %d\n", tid, i, (int)x, (int)y);

       io->readImageBlock2Band(1, pData, x, y, TILESIZE, TILESIZE ,TILESIZE, TILESIZE, kealib::KEADataType::kea_8uint);
       try
       {
           io->getNoDataValue(1, &nodata, kealib::KEADataType::kea_8uint);
       }
       catch(kealib::KEAIOException)
       {
       }
    }

    free(pData);
}

int main()
{
    kealib::KEAImageIO io;
    H5::H5File *h5file = kealib::KEAImageIO::openKeaH5RDOnly("data.kea");
    io.openKEAImageHeader(h5file);

    std::thread t[NUM_THREADS];
    for (int i = 0; i < NUM_THREADS; ++i) 
    {
        t[i] = std::thread(process, &io, i);
    }

    for (int i = 0; i < NUM_THREADS; ++i) 
    {
        t[i].join();
    }

    io.close();
   return 0; 
}