gh-127567: Avoid forceful shutting down of handlers during reconfiguration

[Agent-Hellboy]

• Issue: logging.config.dictConfig() with "disable_existing_loggers=False" closes existing handlers #127567

[aparshin]

@Agent-Hellboy I don't understand how this test covers the problem described in the original issue... The problem in the issue is with handlers of loggers that are not supposed to be modified by new configuration, not with handlers defined in the configuration...

I would propose the following test:

def test_disable_existing_loggers(self):
        fn = make_temp_file(".log", "test_logging-disable-existing-loggers-")
        file_handler = logging.FileHandler(fn, mode="w")
        file_handler.setFormatter(logging.Formatter("%(message)s"))
        root_logger = logging.getLogger()
        root_logger.addHandler(file_handler)

        config = {"version": 1, "disable_existing_loggers": False}

        # we have disable_existing_loggers=False,
        # so, all handlers should continue working
        self.apply_config(config)

        msg = "test message"
        logging.warning(msg)
        file_handler.close()
        with open(fn, encoding='utf-8') as f:
            data = f.read().strip()
        os.remove(fn)
        self.assertEqual(data, msg)

[Agent-Hellboy]

okay, I was testing assert fileHandler in root_logger.handlers only, I have used yours test now

[ZeroIntensity]

You should clarify that this is related to the logging module.

[aparshin]

The reason why now the test is passing is that the root logger is not included in manager.loggerDict(). If you change the following line in the test

root_logger = logging.getLogger()

to

root_logger = logging.getLogger("any-logger")

the test will fail.

[Agent-Hellboy]

sorry, I simply copied your test without giving a thought

[aparshin]

Shouldn't we call this _clearExistingHandlers() after we applied all the modifications from config, not before as it's happening now?

[Agent-Hellboy]

I have added a check

cpython/Lib/logging/config.py

Line 907 in 2041a95

#Remove any existing handlers

here as well to counter it , I will do that for fileconfig as well

[aparshin]

Should we rename the method to better reflect its behaviour? To something like _clearNotUsedHandlers.

[aparshin]

the logging.shutdown() method calls handler.acquire(), handler.flush() and handler.release(). Also, it iterate all these handlers in a specific order (not sure why).

Don't we need all these actions here?

[Agent-Hellboy]

Also, it iterate all these handlers in a specific order (not sure why)

didn't get your question, it is to protect the handler in a concurrent environment. A mutex lock , acquire operation release

[aparshin]

My question is why your implementation in this PR doesn't used them?

[aparshin]

@Agent-Hellboy I see the following issues in this PR:

1. I think that the original issue can be solved without introduction of new config's property remove_handlers. I described my vision of how it can be done in the issue.
2. I don't understand how it's related to the disable_existing_loggers config's property. Either we want to disable exiting handlers or not, the behaviour related to closing handlers should be exactly the same (particularly, we shouldn't close handlers even if we decide to disable them, etc.).
3. This PR is missing the regression test case. If it fixes a problem, there should be a meaningful test case that failed before the PR, and is working fine with this PR.

[github-actions]

This PR is stale because it has been open for 30 days with no activity.