Call Context._logging_fini() in Context.try_shutdown()

[ivanpauno]

Fixes #518 (comment)

[wjwwood]

Is this ok to do with the lock acquired?

I was thinking maybe leave this outside the lock, leave the lock usage in fini logging but make it an rlock?

[ivanpauno]

Calling _call_on_shutdown_callbacks() was supposed to be ok, as try_shutdown() was already doing that.
But we were taking the locks in the opposite order in another place, so that doesn't look fine.
c9cba6a, c2f71b5 should fix it.

I was thinking maybe leave this outside the lock, leave the lock usage in fini logging but make it an rlock?

I prefer only using an rlock when it's actually needed and not because of an implementation detail.
g_logging_configure_lock is always locked after self._lock, so there's no deadlock.
Doing all the shutdown process atomically keeping self._lock held sounds like a good idea IMO.

[sloretz]

It looks like before this PR on_shutdown and shutdown() did not call callbacks with self._lock acquired, while try_shutdown() did.

Calling callbacks with self._lock seems fine to me because the context is already shutdown when it's called. There doesn't seem to be a useful method on a shutdown context that a callback might want to call. However, it looks like that makes self._callbacks_lock redundant. Anywhere self._callbacks_lock is used self._lock is already held.

I would recommend either making try_shutdown() not call callbacks with self._lock, or removing self._callbacks_lock. I personally like the second option because it means one less lock to keep track of when reviewing PRs to Context.

[ivanpauno]

However, it looks like that makes self._callbacks_lock redundant. Anywhere self._callbacks_lock is used self._lock is already held.

That's true

I would recommend either making try_shutdown() not call callbacks with self._lock, or removing self._callbacks_lock. I personally like the second option because it means one less lock to keep track of when reviewing PRs to Context.

Yes, that makes sense to me.
There's not much benefit of having two locks here.

[ivanpauno]

Done in d909a71

[ivanpauno]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows