Add ClearLog functionality for log management#2954
Conversation
WybrenKoelmans
force-pushed
the
truncate-infolog
branch
from
26fefd2 to
5e4b24a
Compare
|
Excuse the force push but I think the review comments should be addressed now. |
- Introduced helper functions for opening log files with UTF-8 BOM and buffering. - Updated log file rotation to ensure proper stream handling and error reporting. - Modified RotateLogFile method to return a boolean indicating success or failure.
| if (!isRegistered) | ||
| return 1; | ||
|
|
||
| FILE* newStream = log_file_openFreshLogFile(filePath); |
There was a problem hiding this comment.
What happens if a log is attempted in between when we close the old stream/set it to nullptr and when we reopen the fresh file?
I think we might need to be careful about concurrency here, if you imagine multiple threads are logging when game code tries to rotate a file?
There was a problem hiding this comment.
I imagine there will be multiple edge cases there:
- separate thread has a handle on oldStream, main thread closes, separate thread tries to write (error or logs lost?)
- separate thread has a handle on oldStream, main thread flushes, separate thread tries to write, main thread closes (logs lost)
- main thread closes/sets nullptr, separate thread gets stream as nullptr, ??? (do we double open? do we error? do we lose the logs?)
- etc.
If you can't tell, I have nightmares from dealing with file writes + concurrency lol.
There was a problem hiding this comment.
In general, because init was handled in main thread on startup before, we didn't really have to worry about concurrency.
But if we're doing these datastructure mutations at the same time we do concurrent LOG()s, we're going to need some sort of synchronization in place. (ie locking/mutexes?)
(for ex. did you know that calling vector.erase(iter) will break other thread's pointers that are doing for (auto iter : vector)? It can make them run off the bounds of the array into memory)
There was a problem hiding this comment.
Practically that probably means making the whole log_file:: namespace thread safe. Which is probably suited for a separate PR to merge before this one? just spit balling.
There was a problem hiding this comment.
nice. would probably need to extend that pr to use the mutex in more places but glad to see there's prior art already.
There was a problem hiding this comment.
Do you (plural) agree that we can probably merge this PR without implementing a mutex for now? Perhaps as it can be done as part of that other linked Issue if really needed.
Use idiomatic C++ instead of raw pointers
|
I don't the thread safety has to be part of this PR, so I'd like to offer it for merging. |
why not? or at least, is it safe enough to prevent engine crashes? |
|
Well im no expert on the multi-threading nature of Recoil, but from my assumption that the Lua will run on a single thread I would think it'll be unlikely to crash the engine. |
I guess what im worried about most is clearlog doing things to log files while background threads (eg pathfinding threads) write logs. I'm not sure whether that is safe against crashes (if it does happen) or never occurs at all. |
sprunk
left a comment
sprunk
left a comment
There was a problem hiding this comment.
If the existing code has no mutex I think it's ok to merge (on the assumption that whatever makes it work correctly will keep doing so) and just yell at you if something explodes.
|
There is definitely some undefined behaviour on truncate/rotate + other threads (eg. sound thread) logging at the same time. Whether that's an issue in practice is a 🤷 depends on when this is called. Let's see if we can land this pr first? Then this PR just needs to grab the mutex in the truncate/rotate bits. #3050 |
|
@bruno-dasilva, I have a question about where we use logging in multiple threads. It may be easier for us to forbid that from happening. |
|
Somebody in the future might want to add logging to debug whatever problem they are facing and I'm fairly sure they will just add |
@lostsquirrel1 two places i saw that emit logs on non-main threads are sound thread and GameServer thread, but there may be more.
Very good point. One option is we can log to a per-thread file... but that removes the convenience of a single infolog. |
|
Note that nearly all logging would still go to the same infolog.txt if we keep that filename for the main thread. The rare logs off the main thread will safely land in a different file not crashing the engine, and still reasonable discoverable. Using a tool like lnav will recombine the logs easily in order (even now I would highly recommend that tool for looking at the logs!) |
5 participants
Introduce a ClearLog feature to manage log files, allowing for both truncation and rotation of logs. This enhancement improves log file handling within the system.
AI usage:
2 shot 100% Claude Opus 4.7