Initial commit to fix log output from AuthzWorker.cpp #1279
Conversation
Reviewer's Guide by SourceryThis pull request fixes a bug where log output was not being written to a file due to the ofstream going out of scope. The fix ensures that the log file is created and that log messages are written to it. Additionally, the fix ensures that cerr is only added once to the logger. Sequence diagram for the fixed logging process in AuthzWorkersequenceDiagram
participant C as Client
participant AW as AuthzWorker
participant L as Logger
participant F as LogFile
C->>AW: checkAuthorization()
activate AW
AW->>L: addStream(cerr) if not added
Note over AW,L: Only adds cerr stream once
AW->>F: open log file
AW->>L: addStream(log_file)
Note over AW,L: Stream gets unique ID
AW->>L: log debug message
AW->>AW: perform authorization check
AW->>L: log result/errors
AW->>F: close log file
AW->>L: removeStream(stream_id)
AW-->>C: return result
deactivate AW
Class diagram showing Logger modificationsclassDiagram
class Logger {
-list~StreamEntry~ m_streams
-mutex m_streams_mutex
-static uint32_t m_stream_id
-LogLevel m_log_level
-bool m_output_to_syslog
+setLevel(LogLevel)
+uint32_t addStream(ostream&)
+removeStream(uint32_t)
+setSysLog(bool)
-output(LogLevel, string, string, int, LogContext, string)
}
class StreamEntry {
+reference_wrapper~ostream~ stream
+uint32_t id
+unique_ptr~mutex~ mutex
+StreamEntry(ostream&, uint32_t)
}
Logger *-- StreamEntry
note for Logger "Changed from vector to list for better add/remove performance"
note for StreamEntry "New class to track streams with unique IDs"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @JoshuaSBrown - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider replacing the static boolean flags (log_stream_added, cerr_stream_added) with a more robust solution like RAII or a proper singleton pattern for managing logger state. Static flags can be problematic for maintenance and in multi-threaded scenarios.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| namespace SDMS { | ||
|
|
||
| static bool log_stream_added = false; | ||
| static bool cerr_stream_added = false; |
There was a problem hiding this comment.
🚨 suggestion (security): Consider using std::atomic for the static flags to prevent race conditions in multi-threaded environments
These flags are accessed from multiple threads and could lead to race conditions. std::atomic would provide the necessary thread safety.
| namespace SDMS { | |
| static bool log_stream_added = false; | |
| static bool cerr_stream_added = false; | |
| #include <atomic> | |
| namespace SDMS { | |
| static std::atomic<bool> log_stream_added{false}; | |
| static std::atomic<bool> cerr_stream_added{false}; |
There was a problem hiding this comment.
This is a good suggestion.
There was a problem hiding this comment.
Thank you for your feedback, we will generate more comments like this in the future.
| @@ -638,40 +641,77 @@ const char *getReleaseVersion() { | |||
|
|
|||
| // The same | |||
| int checkAuthorization(char *client_id, char *object, char *action, | |||
| struct Config *config) { | |||
There was a problem hiding this comment.
issue (complexity): Consider using RAII with a LoggerGuard class to simplify log stream management and eliminate global state.
The current implementation adds unnecessary complexity with global state and manual stream management. This can be simplified using RAII:
class LoggerGuard {
std::ofstream log_file;
uint32_t stream_id;
bool stream_added;
public:
LoggerGuard(const std::string& path) : stream_added(false) {
if (!path.empty()) {
log_file.open(path + "_authz", std::ios::app);
if (log_file.is_open()) {
stream_id = SDMS::global_logger.addStream(log_file);
stream_added = true;
}
}
}
~LoggerGuard() {
if (stream_added) {
SDMS::global_logger.removeStream(stream_id);
}
}
};
int checkAuthorization(char *client_id, char *object, char *action, struct Config *config) {
static std::once_flag cerr_flag;
std::call_once(cerr_flag, []() {
SDMS::global_logger.addStream(std::cerr);
});
// Logger setup
SDMS::global_logger.setSysLog(!defined(DONT_USE_SYSLOG));
SDMS::global_logger.setLevel(SDMS::LogLevel::INFO);
LoggerGuard log_guard(config->log_path);
// Rest of the authorization logic...
}This approach:
- Eliminates global state variables
- Automatically handles stream cleanup
- Uses std::call_once for thread-safe cerr initialization
- Reduces nesting and improves readability
There was a problem hiding this comment.
This is also a good suggestion.
There was a problem hiding this comment.
So on looking at this closer there remain some problems. for 1 if multiple threads are writing to the same file will it cause problems if I close the file when the checkAuthorization command goes out of scope, but other threads may be attempting to write to the file. It looks like the member log_file is specific to the thread instance, but I don't know if closing a file stream changes the actual behavior of the file as seen from separate threads.
There was a problem hiding this comment.
You're right to be concerned about multiple threads writing to the same file. Closing the log_file stream in the destructor of LoggerGuard while other threads might still be writing to it could lead to issues, particularly if those threads attempt to write to the file after the stream has been closed. This could result in lost data or unexpected behavior.
Here are a few ways to address this:
-
Thread-specific log files: The simplest solution is to have each thread write to its own log file. You can modify the
LoggerGuardconstructor to incorporate thread-specific information into the filename (e.g., usingstd::this_thread::get_id()). This eliminates the contention issue entirely. -
Shared mutex for file access: If you need to write to a single shared log file, you'll need to synchronize access to the file stream using a
std::mutex. You can encapsulate thestd::ofstreamand thestd::mutexwithin theLoggerGuardclass. TheLoggerGuardconstructor would acquire the mutex, and the destructor would release it after closing the stream. This ensures that only one thread can access the file stream at a time. However, this introduces a performance bottleneck as threads will have to wait for the mutex. -
Asynchronous logging with a dedicated logging thread: For optimal performance, consider using a dedicated logging thread. The
checkAuthorizationfunction would push log messages onto a thread-safe queue. The logging thread would then consume messages from the queue and write them to the log file. This approach decouples logging from the main thread and avoids blocking. This is more complex to implement but offers the best performance. -
Use a logging library: Consider using a dedicated logging library like spdlog, log4cxx, or Boost.Log. These libraries typically handle thread safety, file rotation, and other logging-related concerns efficiently. This is often the most robust and easiest long-term solution.
Given the potential complexity of multi-threaded logging, I strongly recommend option 4, using a well-established logging library. It will save you time and effort in the long run and provide a more robust solution. If that's not feasible, option 1 (thread-specific log files) is the simplest to implement and avoids the performance overhead of synchronization. Options 2 and 3 offer increasing performance benefits but come with increased implementation complexity. Choose the option that best balances your performance needs and development resources.
PR Description
Fixes bug where ofstream was going out of scope and no log output was going to file from cpp AuthzWorker. With this fix the file is being created and we are seeing the following result.
Tasks
Summary by Sourcery
Bug Fixes: