Btw, you might be able to use TLazThreadedQueue from lazcollections instead of TLogQueue.
Otherwise, you now have a separate thread just for logging.
That means, it does still carry the risk of something not being logged, albeit much smaller chance.
Unless I read something wrong:
- you add the text to the queue
- you signal the logger thread that something is there.
- you continue (i.e., the calling thread does not wait, until the logger thread has logged)
My understanding is, that if (in the small time, until the logger thread does its work) there is a fatal error and the app is terminated (shut down by OS, no waiting for threads), then the logger thread may be terminated before it logs.
Its a small chance, but it is there.
Also, you open and close the logfile each time.
So you could just but the TMPLogger.Log into a critical section and execute it within each thread.
Sure then the threads have to wait... But if you need 100% "no log lost" then the thread need to wait for the logger thread too. Otherwise the logger thread can always be force terminated while it has unlogged data.
And sorry for seemingly tearing your solution apart... But only trying to help.
There is another issue.
If I understand correctly, then "LogLn" is called by the threads?
So it could be called by multiple threads, and you can have a race condition on "if not Assigned(GlobalMPLog) then", leading to more than one logger running.
You could instead of "GlobalMPLog := TMPLogger.Create(ThisFileName);"
do something like
if GlobalMPLog <> nil then begin // test for most cases / deal with race condition below
tmp := TMPLogger.Create(ThisFileName); // starts a new logger, even if not needed
old := InterlockCompareExchange(GlobalMPLog, tmp,nil); // only sets the new value, if GlobalMPLog is nil / thread save
if old <> nil then // if GlobalMPLog was not nil => clean up
tmp.free; // and shut down the thread...
end;
After that use GlobalMPLog which either has the correct logger.
Your SetFilename has a critical section => so do you change the filename while the logger is running.
(and therefore only one thread at a time is allowed to set it?)
You don't use a critical section in TMPLogger.Log when you read the filename.
AnsiStrings are in many cases threadsafe, but not always. This particular instance may not be...
Because MyFileName has a refcount of 1. (At least it could end up with that).
Then while you change it, it may be dec-refed and freed.
But in a race condition that "free" could happen while the logger-thread tries to get a inc-ref on it. The logger thread would incref a dangling pointer, and then use freed mem as the string.
IMHO its as likely as the one person winning the lottery jackpot twice in a row, but it is possible.
=> If you assign the global var (the field in the global logger counts as global) to a local var, while in a critical section, then the local var is thread save afterwards. Even if the global var is changed (and even if the global var points to the same data). This is because the local var has a ref to the string that is owned only by one thread....
Btw, the "log in critical section" exists in LazLogger, though needs some setup.
Using TLazLoggerFileHandleThreadSave gives the critical section
Using CloseLogFileBetweenWrites will ensure the file is opened and closed each time.