rather than disabling the logging, can we instead append the record to aself._reentrant_records = collections.deque(), and then process all of the pending records:

maybe_record=self.filter(record)ifnotmaybe_record:returnifisinstance(maybe_record,LogRecord):record=maybe_recordwas_calling_handlers=set_calling_handlers()try:ifnotwas_calling_handlers:self.callHandlers(record)whileTrue:try:record=self._reentrant_records.popleft()exceptIndexError:returnself.callHandlers(record)else:self._reentrant_records.append(record)finally:set_not_calling_handlers()

This will still produce a stack overflow if handling the deferred log message itself logs another message

I'm not following how this would cause a stack overflow, if handling the log message logs another message it would go onto the _reentrant_records queue, and then be processed later once the stack returns all the way back to whereset_calling_handlers() is first called.

I'm not following how this would cause a stack overflow, if handling the log message logs another message it would go onto the _reentrant_records queue, and then be processed later once the stack returns all the way back to whereset_calling_handlers() is first called.

Sorry, I should have said deadlock with the current example. The stack overflow is from a different way of triggering this (see the second unit test added).

The trouble is that when the first recursive logging call exits thefinally block it clears the "calling handlers" flag, which means a subsequent (still recursive) one takes the wrong path and deadlocks/overflows.

That can be avoided for the original triggering example by only clearing the "handling" flag if it was initially unset (the deferred records collection also needs to be TLS not a member variable). It ends up looking something like this:

ifnothasattr(self._tls,'reentrant_records'):self._tls.reentrant_records=deque()deferred=self._tls.reentrant_recordswas_calling_handlers=self.set_calling_handlers()try:ifnotwas_calling_handlers:self.callHandlers(record)whiledeferred:self.callHandlers(deferred.popleft())else:deferred.append(record)finally:ifnotwas_calling_handlers:self.set_not_calling_handlers()

This fixes the two bugs, which only log thefirst time they try to process a log record (and means those recursive log messages are logged and not silently ignored, which is nice). However a different example which logsevery time (such as the second unit test) will still live-lock and never exit thatwhile loop.

will still live-lock and never exit that while loop.

Does the system keep logging forever instead?

This seems acceptable as you'd easily track down this bug just by looking at the logs

Does the system keep logging forever instead?

Yep, it will just sit in a tight loop spamming the log forever, or at least until/unless that exhausts disk space or wherever the logs are actually going.

This seems acceptable as you'd easily track down this bug just by looking at the logs

IMO it is not agreat failure mode, but it will certainly be obvious!

FWIW I think I prefer ignoring them: the code is much simpler and it prevents the issue in non-trivial handler implementations like Sentry's (that would otherwise trigger the live-lock failure). I was hoping this fix would mean they would be able to remove that nasty monkey-patching on supported versions.

OTOH it is definitely nice to actually handle instead of drop the recursive log messages, in cases where they don't always continue to recurse.

we could have alogging.(_)N_RECURSIVE_CALLS constant to limit this so it's not forever

We could. Another alternative would be Victor Stinner's suggestion in the discourse discussion to raise an exception. That would bring it to the user's attention and force them to deal with it.

Ultimately, though, the way they will have to deal with it is by preventing, disabling, or otherwise intercepting and ignoring all such logging. That will be difficult to do reliably outside the core, will likely be overlooked unless/until it bites, and have to be done in every susceptible log handler or application that uses such.

IMO it would be better for us to do this once, centrally, with a small, simple, and robust fix.