RFC: Change to how system header threads are started

[oBFusCATed]

This is about the Code completion plugin.

I've recently started running C::B with Address sanitizer (asan).
I've found numerous leaks and I'm trying to fix them as they are found.

One such leak was when cleaning the system headers threads during shutdown. There was a missing delete of the thread object. I've fixed this in rev 11475.

Since then I've found that sometimes C::B deadlocks during shutdown. After some debugging I've discovered that this new call to delete causes the dead lock. I'm not really sure if this isn't a wx issue (running both 2.8, 3.0.4 and some master), but I don't have the desire to find out.

It turns out that the code related to the system headers thread management is really strange. For some strange reason new threads are created but not started immediately. They are only started after the parsing is done. But I don't understand why! Can someone explain why the code is written in this manner?

It seems that calling delete on a non started and not waited thread blocks and there is no one to unblock it.

I've prepared a patch which just starts the threads immediately. See it attached. After minimal testing I couldn't find problems. If someone thinks there is a reason the code is done like it is, please speak now. I'll do a bit more testing and I'll commit it.

[MortenMacFly]

Well these portions s are really old and partially contributed. I believe the reason was that the systems headers threads should use parts of what has been parsed and/or because the order matters (remember: The order can be setup in the settings). So, first parse what header you'll need to parse, then fix the order and then parse the headers in that particular order.

However, I am not sure if that is still relevant becasue for example we have changed macro handling in the mean time. My suggestion therefore: Lets test your patch and see for some times if it has side-effects.

[MortenMacFly]

...reminds me: Another work-around would be to check if the thread is actually running/active before delete. This is probably recommended anyway because of the deadlock. I am thinking about scenarios like of quick shut-down after start where this could be an issue.

For sure this will be platform dependent.

[oBFusCATed]

The checks were already there and they failed. You can see that I've removed them.

As far as I can see this code is only to gather the list of threads to show for #include completion. I couldn't see any other code to use this list. But I might have missed something.

[MortenMacFly]

As far as I can see this code is only to gather the list of threads to show for #include completion. I couldn't see any other code to use this list. But I might have missed something.

OK - looks like that, indeed. The name is really misleading. So, lets give it a try... Its compiled in my local version.

[ollydbg]

It turns out that the code related to the system headers thread management is really strange. For some strange reason new threads are created but not started immediately. They are only started after the normal parsing stage is done. But I don't understand why! Can someone explain why the code is written in this manner?

The name "system header thread" is quite bad, and I suggest a better name like: "header file crawler".

This part of code is independent of the normal parsing code. It just does what a file crawler does, that is collecting header files under user defined include search paths or compiler predefined include search paths.

The code is contributed by our developer Loaden(he is not active right now) several years ago. The reason those threads are started only AFTER the normal parsing stage done is that we don't want to cost CPU to much when we are in normal parsing stage. Since normal parsing stage are running thread tasks from a thread pool, thus, Loaden just deferred the header file crawler until the normal parsing stage is done.

Hope that helps. 

[oBFusCATed]

Aren't we parsing source code in a single thread at the moment? Or is it a thread per project/parser?

[MortenMacFly]

Just for you notice: Seems to work so far on Windows without issues. I also don't see any significant delays, but I have a SSD that the crawler scans. So this might experience be biased.

[oBFusCATed]

I don't think we'll see performance regressions from this. Probably on very slow machines, but they would be just slow anyway. On the contrary I think the list of includes would be available a lot sooner...