Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Possible Bug] : undesired deletion of logger from registry(loggers_ map) #3016

Closed
raj17ce opened this issue Feb 18, 2024 · 2 comments
Closed

Comments

@raj17ce
Copy link

raj17ce commented Feb 18, 2024

Steps to produce

  1. Created a shared pointer of logger class and added it to registry.
std::shared_ptr<spdlog::logger> myLogger = std::make_shared<spdlog::logger>("myLogger");
spdlog::register_logger(myLogger);
  1. Set this logger to default logger
spdlog::set_default_logger(myLogger);
  1. Create another shared pointer to a new logger object and set it default.
std::shared_ptr<spdlog::logger> myLogger2 = std::make_shared<spdlog::logger>("myLogger2");
spdlog::set_default_logger(myLogger2);
  1. when trying to access the first pointer(myLogger) from registry using get("myLogger") method, it returns nullptr.
auto myLogger = spdlog::get("myLogger");

This is undesirable, as I registered myLogger into the registry. But changing the default logger not only changed the default logger but also deleted it from the registry map loggers_ .

I tried to track the issue, and I found that while setting another default logger, the entry for the previous logger is erased from the registry map loggers_.

[File] : registry-inl.h
[Function] :

SPDLOG_INLINE void registry::set_default_logger(std::shared_ptr<logger> new_default_logger) {
    std::lock_guard<std::mutex> lock(logger_map_mutex_);
    // remove previous default logger from the map
    if (default_logger_ != nullptr) {
        loggers_.erase(default_logger_->name());
    }
    if (new_default_logger != nullptr) {
        loggers_[new_default_logger->name()] = new_default_logger;
    }
    default_logger_ = std::move(new_default_logger);
}

Inside the first if block, the entry is being erased.

@gabime
Copy link
Owner

gabime commented Feb 18, 2024

I agree. Seems undesirable.Maybe the fix would be just set the default_logger_ without touching the loggers_ at all. I can’t recall though why it was implemented like this.

@raj17ce raj17ce closed this as completed Feb 19, 2024
@raj17ce raj17ce reopened this Feb 19, 2024
@raj17ce
Copy link
Author

raj17ce commented Feb 19, 2024

Should I create a Pull Request removing the below if condition ??

if (default_logger_ != nullptr) {
        loggers_.erase(default_logger_->name());
}

@gabime gabime closed this as completed in 3403f27 Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants