ENH: Make logger thread-safe

[ntatsisk]

No description provided.

[N-Dekker]

Thanks for your suggestion, @ntatsisk Honestly I had forgotten about thread_local 😺 ! I do hesitate, though. This would mean that each and every thread would have its own "global" xoutmain object. Marius previously suggested to have an xoutmain object for each elastix filter instance. In either way, we would then loose the ability to manage the console output. Right? I think it would make sense to ensure that each single line of output would be from one specific thread.

BTW, There's a segfault from GitHubActions/ubuntu-18.04, do you happen to know why?

2021-02-07T22:40:30.0524868Z 157/158 Test #157: CommonGTest_test ........***Exception: SegFault 0.58 sec

To be continued...!

[ntatsisk]

Indeed, the console output becomes unreadable because all threads write unsychronized there but, on the positive side, it works without crashing. In general, that doesn't seem as a problem to my eyes. The reason is that even if each line of the output belonged to a single thread, it wouldn't be helpful either. Imagine 12 or 24 threads taking turns to write just a single line, how could somebody make something out of it? On the other hand, since the log files work, the user could use those if debugging was necessary. That's how I see it!

Regarding the segfault, it happens in the ElastixMain.GetComponentDatabaseAndCreatorInParallel test, right? I have no clue why this happens. When I try the tests locally on a windows machine, all tests pass. I will look into it further.

[ntatsisk]

Hi @N-Dekker. I tried to make some comparison between MSVC and MinGW in respect to thread_local. As I mentioned, compiling with MSVC passes all tests and the final project works fine. For the case of MinGW, I noticed that GetComponentDatabaseAndCreatorInParallel from CommonGTest pass which seemed to give SegFault in linux. However, when I try to run the ElastixLibGTest it hangs forever in the first test. That also happens when I remove the thread_local keyword so I assume that something is off with my MinGW build.

In conclusion, I still don't know what caused the original SegFault in the linux build and, unfortunately, I don't have an easy way to debug it. Any ideas on how we could proceed? One xoutmain per elastix filter or a single shared xoutmain?

[N-Dekker]

@ntatsisk Hi Konstantinos, thanks for your MSVC/MinGW comparison.

Any ideas on how we could proceed? One xoutmain per elastix filter or a single shared xoutmain?

As you said before that the console output becomes unreadable when multiple threads write to the console via xout, I wonder, why should that be supported anyway? Shouldn't multi-threading users just do LogToConsoleOff()?

[ntatsisk]

@N-Dekker, yes I agree LogToConsoleOff() is what makes sense, especially if you consider that the user uses multithreading for performance reasons. I just mentioned that although the output is mixed, it doesn't throw any errors which seemed as a positive thing to me. Could the strategy of a xoutmain object per elastix filter be implemented without thread_local? Or should we focus debugging the current PR on linux? Because on windows it seems to work fine.

[N-Dekker]

@ntatsisk Do you have the Linux SegFault even after commit ef55738 (9 February 2021)? The removal of xout["iteration"] should at least slightly ease multi-threading support.

[ntatsisk]

@N-Dekker, It looks that it passed the tests. However, I think it is very unlikely that the xout["iteration"] did the job. One reason is that the threads are supposed to hold their own xoutmains and, also, from what I understand, the GetComponentDatabaseAndCreatorInParallel test that was failing before doesn't involve any writing of iterations.

Can we run the gtest that you wrote in #393 to see if it can pass now?

[ntatsisk]

I added the gtest of #393 in the current PR. However, only the "Elastix" check was triggered, if I am not mistaken. Is there a way to repeat the other checks for the different OS builds? Moreover, what additional tests/changes would be necessary so that this PR makes its way to the develop branch? Thanks.

[ntatsisk]

I am closing this one, since #429 solved the issue of multi-threading at user level.

[N-Dekker]

I am closing this one, since #429 solved the issue of multi-threading at user level.

Thanks, @ntatsisk I'm glad to hear that pull request #429 appears to address the issue 😃