ImagingDataCommons / libdicom

C library for reading DICOM files
https://libdicom.readthedocs.io
MIT License
16 stars 7 forks source link

add dcm_init() startup function #38

Closed jcupitt closed 1 year ago

jcupitt commented 1 year ago

to make libdicom threadsafe, resolves https://github.com/ImagingDataCommons/libdicom/issues/19

bgilbert commented 1 year ago

It looks like dcm_init() itself isn't thread-safe, which makes this unsafe to call from other libraries except from their shared library constructors. (There might be two threads, where one calls library A which calls dcm_init() and the other calls library B which calls dcm_init().)

We could instead make this a shared library constructor, or call it on first use via a GOnce-style primitive. I guess we don't want to add the dependencies for either approach?

jcupitt commented 1 year ago

I agree, I don't think a pthreads dependency is a good idea, which I think something like GOnce would need.

We could use __attribute__((constructor)), though I suppose we'd still need to expose dcm_init() in case the compiler or platform had issues with it.

hackermd commented 1 year ago

@jcupitt we initially implemented the dictionary using a static lookup table. Given the challenges with making dcm_init() threadsafe, would it make sense to reconsider this approach?

jcupitt commented 1 year ago

I think the constructor tag should be simple and safe 99% of the time. The code is now:

/**
 * Start up libdicom.
 *
 * Call this from the main thread during program startup for libdicom to be
 * threadsafe.
 *
 * If you don't do this, libdidom will attempt to call it for you in a safe
 * way, but cannot guarantee this on all platforms and with all compilers, and
 * therefore cannot guarantee thread safety.  
 *
 * This function can be called many times.
 */
DCM_EXTERN
DCM_CONSTRUCTOR
void dcm_init(void);

So the init function will usually be called automatically when the library is loaded. If a threaded application wants to be 100% safe, it needs to arrange to init the library as part of its own startup. This is a pretty standard and reasonable requirement and I think expected behaviour.