bpo-37857: Invalidate cache when log level changed directly

[zaneb]

bpo-37857

While the logging level of a Logger should really be changed only by calling the setLevel() method, there exists code in the wild that changes it by setting the public level attribute directly. Prior to
Python 3.7, this worked correctly.

The addition of caching to Logger.isEnabledFor() by the fix for bpo-30962, #2752, means that direct changes to the level now leave the cache in a state inconsistent with the Logger - an extraordinarily difficult thing to debug.

Make level a property that invalidates the cache when set. This ensures that existing application code will continue to work as it did with previous versions of Python.

https://bugs.python.org/issue37857

[epicfaace]

Looks good!

[vsajip]

Please see my comments on the issue.

[zaneb]

@vsajip what if we added a DeprecationWarning to the setter? That would help people find and fix their old code, and avoid encouraging more folks to try using the wrong interface.

[avylove]

I'm not sure if this is needed or not, but there are a couple things I don't like about the approach. Primarily because it's inconstant. I would suggest changing all level references to _level and leave self.manager._clear_cache() in setLevel() since that is the documented interface.

Since Logger.level is not documented, simply switching to Logger._level will not break the published API. However, those accessing Logger.level would not get a failure, they would simply create a new attribute called level and continue to operate at the previous level. This pretty similar to the state we have now.

I think a reasonable compromise might be to change self.level to self._level and make level a read-only property. That would raise AttributeError: can't set attribute when someone tried to set it directly. The traceback should make it obvious this is referring to level. @vsajip, does this seem workable? If this isn't a sufficient error, a non-functioning setter could be introduced that raises a more descriptive exception.

One could argue a deprecation period is needed and a setter could be used to do that. I wouldn't object to that, but I also would argue it isn't needed since it's not documented and already doesn't work in 3.7+. If you did want to introduce a functioning setter for level, it should be independent of the rest of the code, i.e. if you delete it, everything else still works. Basically, the setter would simply call setLevel().

[avylove]

You're still using level in getEffectiveLevel() rather than _level. This inadvertently makes getEffectiveLevel() slower by requiring an additional method call. Since it's part of the same class, it should use _level.

[csabella]

Based on the discussion on the bug tracker, it looks like @vsajip wasn't in favor of this change. I'm going to close this PR for now with the understanding that it can be revived if this becomes the path forward in the future. Thank you.

[zaneb]

Based on the discussion on the bug tracker, it looks like @vsajip wasn't in favor of this change.

I did make the changes suggested by @avylove to address those concerns, but @vsajip has not commented on them.