@LeonarddeR Ah yes, I should have mentioned I removed the limitation around not logging debug level to NVDA. This limitation was originally there as there are so many debug calls and they would majorly slow down an app's main thread (say when debuggind displayModel) as each call would contact NvDA from that thread. Now as it is in a separate thread it shouldn't be so bad. However, note that by default (and therefore in all official builds of NVDA) the nvdaHelperLogLevel is lower than debug, and therefore the LOG_DEBUG macro is a no-op anyway. In other words, even now, to get messages at level debug, you still need to compile NvDA with nvdaHelperLogLevel at debug or higher.

@LeonarddeR I have fixed the indentation now.

@feerrenrut thanks for the useful review. I think the main reason why I had maxCount as an argument in the first place came from the fact I was trying to avoid an extra function and using log_flushQueue also as the apc function.
I have refactored things based on your feedback. Now:

• log_flushQueue no longer takes a maxCount argument. It just flushes the number of items available in the queue. Note though it does still only call queue.size once at the beginning, one wouldn't want the possibility that another thread kept adding to the queue while flushQueue was fetching and it just never finished.
• There is now a dedicated apc function which just calls log_flushQueue.
• log_terminate has been removed and log_flushQueue is used instead.
• log_flushQueue is also now called near the top of the manager thread once to fetch any previously queued log messages before the manager thread was started.
  I hope this addresses all of your feedback.

Yes, these changes look good.

Note though it does still only call queue.size once at the beginning, one wouldn't want the possibility that another thread kept adding to the queue while flushQueue was fetching and it just never finished.

I need to look into how APC works to really make assertions around this, but if these two functions can run concurrently then I'm concerned that there should be concurrency control on on this deque. Check the thread safety header on the following page also: https://en.cppreference.com/w/cpp/container

I'll come back to this.

certainly two threads could push to the queue at the same time

I think we will have to protect this deque from this. See the detailed rules under the "thread safety" header on the following page also: https://en.cppreference.com/w/cpp/container

The gist of it is that reading from the container (eg const member functions) are fine, modifying the container not so.

There seems to be two thread safety problems:

• Flush can happen at the same time as items being added, these both modify the queue.
• Two threads could add items to the queue at the same time.

To prevent this we need to introduce locks. Preventing multiple threads from modifying the queue. The main concern is to watch for deadlocks, though data races should be kept in mind. By ensuring we only hold a lock while making changes to the queue, we can prevent deadlocks. The trick to this is localizing the code that needs a lock.

So, I see two options:

1. Use two queues (it may no longer make sense to use deque specifically, but that is another concern). Have an active receive queue, and an inactive queue. When flush is called a lock is acquired just while they are swapped. Flush then empties the inactive queue. The same lock is acquired in log_message to ensure the active / inactive queues aren't swapped during the insertion/append/push.
2. Continue with a single queue, but use a lock to protect during insert, and during the flush. While you could lock for each call to pop in flush, I'm not really comfortable to recommend it. Instead, after carefully considering the thread safety rules for STL containers, it might be ok to iterate through the elements logging them (up to maxCount) then lock and remove them in one call.

Assumptions:

• Flush is only executed on the inproc Manager Thread, never concurrently. It would be good if we could ensure this by checking at the start of flush, and if not queuing it with APC and exiting early.

@feerrenrut in log_flushQueue I now lock and swap. And in logMessage I now also lock.

• Build (for testing PR)

See test results for failed build of commit 5809d3a675

@feerrenrut I committed your suggestions but made a couple of small changes: We can only queue via APC if the manager thread actually exists yet. And log_flushQueue_apcFunc needed to be declared first. Does this all look okay now?