Bug 6607 – critical_.d and critical.c use double check locking the wrong way

First of all, I'm quite new here and hope I'm not plein wrong. If this is please forgive me. I was studying the druntime to know more about what's going on behing the hood of D2, and I found out that synchronized may have been implemented the wrong way. Consider this piece of code from critical_.d : if (!dcs.next) { pthread_mutex_lock(&critical_section.cs); if (!dcs.next) // if, in the meantime, another thread didn't set it { dcs.next = dcs_list; dcs_list = dcs; pthread_mutex_init(&dcs.cs, &_criticals_attr); } pthread_mutex_unlock(&critical_section.cs); } The double check locking patern is used the wrong way here. dcs.next may have been set before the whole dcs struct has been initialized, thus leading to undefined behaviour. Reordering instruction within the second if isn't suffiscient as long as compiler, or CPU may reorder instruction and memory read/write. The same goes for window code, which is very similar, and c code within critical.c . I'm not sure which one is used and when). This code duplication should eventually be reduced/suppressed in the process to avoid having a problem poping in several place like we can see now. I would be happy to help to resolve that problem, so don't hesitate to contact me. Please be indulgent if the problem is irrevevelant because I didn't understood the way druntime worked. I'm currently learn, and, even if I have a strong background in devellopement even at low level, druntime is really new to me and I'm still at the stage where I'm learning it.

I've always hoped that because Walter implemented this module, he did so knowing how DMD would generate the involved code. I do agree that this isn't a safe long-term assumption though, since the compiler may change, leaving the need to update this code as a forgotten detail. What should really happen is for all the .c modules to be transitioned to .d modules, and for any questionable practices like these to be changed to something more universally reliable. Between new language features and core.atomic I think this can be improved without actually sacrificing performance.

Actually, even if the compiler do not reorder that, modern CPU can, and this is out of control, unless specific precautions are used. It's even more problematic now that almost everybody has a multicore CPU. We need stronger guaranties to make the double check locking pattern valid, for exemple guaranties that java's 5+ volatile keyword provide.

How ludicrous to have a known race condition in synchronized and not fixing it for 3 years.

Commit pushed to master at https://github.com/D-Programming-Language/druntime https://github.com/D-Programming-Language/druntime/commit/0b41a6205361382930272813a5f6a2baae0c456a cleanup and rewrite rt.critical_ - reuse mutex declarations from rt.monitor_ - fix race condition (incorrect double checked locking) - modernize code for readability - use shared(D_CRITICAL_SECTION)* when dealing with unsynchronized data - replace STI/STD style C constructors - fix Issue 6607 - hopefully fix Issue 14584 - spurious autotester deadlocks

Commit pushed to stable at https://github.com/D-Programming-Language/druntime https://github.com/D-Programming-Language/druntime/commit/0b41a6205361382930272813a5f6a2baae0c456a cleanup and rewrite rt.critical_

Thumb up !

Commit pushed to dmd-cxx at https://github.com/dlang/druntime https://github.com/dlang/druntime/commit/0b41a6205361382930272813a5f6a2baae0c456a cleanup and rewrite rt.critical_