fix DefaultMessageFactory thread-race issue #865
Conversation
When multiple threads race to init DefaultMessageFactory, fix it so that later threads do not return before the first thread finishes loading all the dlls
And restore/update the ctor that I deleted in the previous commit but then changed my mind about
|
|
||
| var dlls = Directory.GetFiles(directory, "QuickFix.*.dll"); | ||
| foreach (var path in dlls) | ||
| _dllsAreLoaded = true; |
There was a problem hiding this comment.
I think for ultimate thread safety, either _dllsAreLoaded needs to be marked with volatile or the first if before the lock needs to be removed (easiest to reason about). I think otherwise there is nothing theoretically preventing this write being executed out of order and becoming observable (by another thread outside the lock) before the preceding code has finished.
Pretty theoretical and I may be wrong as to whether that can actually happen
There was a problem hiding this comment.
I think you're right that volatile is probably appropriate.
I was about to argue in favor of leaving the if in, but I realized that this function won't be called enough for that to cause any actual efficiency gain. Not sure why I didn't realize that when I wrote it.
|
Oh, dang, I wish I'd gotten this into 1.12. I forgot that it was still open... |
|
Was hoping @baffled would take a look, but I guess he's not going to. Time to merge... |
|
@Rob-Hague I made the fix to remove the first "if". With that, the "volatile" modifier is probably not needed (which you kind of implied with your comment). |
|
Agreed, volatile not needed, looks good to me |
It was planned to go into 1.12.0, but didn't make it
fix DefaultMessageFactory thread-race issue
It was planned to go into 1.12.0, but didn't make it
move connamara#864/connamara#865's release note to after 1.12.0
When multiple threads race to init
DefaultMessageFactory, fix it so that later threads do not return before the first thread finishes loading all the dlls.resolves #864
attn @baffled