bpo-17852: Revert incorrect fix based on misunderstanding of _Py_PyAtExit() semantics

[pitrou]

The fix committed in #3372 uses _Py_PyAtExit. Unfortunately this function does not add a new callback, it merely replaces the current one. In other words, _Py_PyAtExit is only meant to be called by the atexit module and nothing else.

https://bugs.python.org/issue17852

[pitrou]

@nascheme

[vstinner]

bpo-17852: Revert incorrect fix based on misunderstanding of _Py_PyAtExit() semantics #4826

Would you mind to explain what is wrong? What is the incorrect fix? Are you talking about the commit 0a1ff24 made by @nascheme?

[pitrou]

Would you mind to explain what is wrong?

Please read the PR description :-)

[vstinner]

Please read the PR description :-)

The PR description isn't very helpful. I undertand that Neil implements the change "Maintain a list of BufferedWriter objects. Flush them on exit." but that it doesn't work sometimes. Would you mind to explain when it doesn't work?

Sadly, it seems like the commit 0a1ff24 didn't add a new test, so it's easily see how to trigger a potential bug.

[pitrou]

Would you mind to explain when it doesn't work?

It's simple: both _io and atexit call _Py_PyAtExit when initialized, but that function overwrites a single callback. So both modules end up fighting for having their callback executed at shutdown :-)

(if you don't understand, I really suggest you study the code)

[vstinner]

It's simple: both _io and atexit call _Py_PyAtExit when initialized, but that function overwrites a single callback. So both modules end up fighting for having their callback executed at shutdown :-)

Would it be possible to write a test for that, to prevent regression?

[pitrou]

Would it be possible to write a test for that, to prevent regression?

We could add an assertion rather than a test...

[vstinner]

I understand that if the _io module wins, the atexit callbacks are never called. It should be possible to write a test to ensure that atexit callbacks are called, no?

test_atexit only has unit tests calling directly atexit._run_exitfuncs() and so not really testing the atexit feature. Maybe we need at least one unit test in test_atexit checks the a print("at exit") is executed... at exit, using a subprocess?

Here I understand that atexit always wins, and so Neil's fix never works. Yet another reason for write a test checking that all files are flushed at exit :-) I know how hard it is to make such tests reliable, since Python shutdown process is a mess :-(

[pitrou]

Maybe we need at least one unit test in test_atexit checks the a print("at exit") is executed... at exit, using a subprocess?

Should I add it as part of this PR?

[vstinner]

Should I add it as part of this PR?

Technically, I would prefer to see it in a different commit, instead of hidden in a "revert" commit. So another PR would be better, and it would allow to backport the new test.

[pitrou]

Ok, so I would like to merge this PR soon.

[vstinner]

LGTM.

Ok, I understood the bug.

I don't think that this revert means that it's not possible to flush files at exit. Just that the fix was wrong, and that we need more time to design the fix properly.

[vstinner]

@pitrou wrote the PR #4828: non-regression test for test_atexit.

[pitrou]

I expect the correct fix will to call the atexit.register function.

[nascheme]

I think I have a proper fix, uses the atexit module. Will create a PR shortly.