classification
Title: Replace stdout and stderr with simple standard printers at Python exit
Type: Stage: resolved
Components: Versions: Python 3.6
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: Nosy List: serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-03-25 13:13 by vstinner, last changed 2017-07-26 02:58 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
replace_stdio.patch vstinner, 2016-03-25 13:13 review
replace_stdio-2.patch vstinner, 2016-04-01 21:08 review
Messages (8)
msg262436 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-25 13:13
The Python shutdown process is complex and fragile.

It was discussed in the issue #25654 to replace stdout and stderr with simple "standard printers" (implemented in C, don't depend on other Python objects or modules).

Attached patch implements this idea. The patch begins with closing standard stream objects (sys.stdin, sys.stdout, sys.stderr), and then replace stdout and stderr.
msg262455 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-25 20:54
There are some questions.

1. Is there a reason only name is closed, not dunder_name? (Josh's question, but I'm interesting too).

2. Is it worth to first replace standard streams with "standard printers", and then close original streams? This allows to log warnings from closing streams.

3. "standard printers" are used at startup and at shutdown. Can we reuse some code? Or save and reuse "standard printers"?

4. Daemons close standard streams and fileno(stdout) can return unrelevant value. Perhaps it is not good idea to recreate closed stdout.
msg262456 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-25 21:31
Serhiy Storchaka added the comment:
> 1. Is there a reason only name is closed, not dunder_name? (Josh's question, but I'm interesting too).

By default, sys.__stdout__ is sys.stdout. Is it ok to close the same file twice?

> 2. Is it worth to first replace standard streams with "standard printers", and then close original streams? This allows to log warnings from closing streams.

Yeah, I tried this locally after sending my patch. I think that we
should keep a strong reference and only "decref" after the stream is
replaced.

> 3. "standard printers" are used at startup and at shutdown. Can we reuse some code? Or save and reuse "standard printers"?

I will check, it's maybe possible to share some code inside pylifecycle.c.

> 4. Daemons close standard streams and fileno(stdout) can return unrelevant value. Perhaps it is not good idea to recreate closed stdout.

Ah yes, while playing with my patch, I noticed that I replaced stdout
even if sys.stdout was NULL or None.

Closed stdout is a different thing. I guess that we can try to call
stdout.closed() and only replaced it with a standard printer it
closed() returns True (don't do it on error nor if closed() returns
false).
msg262772 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-01 21:08
Patch version 2:

* check if the stream was already "closed" (see below and comment in the patch)
* first replace stream and then close it and DECREF the object
* don't close stdin anymore


> 1. Is there a reason only name is closed, not dunder_name? (Josh's question, but I'm interesting too).

Fixed.

> 2. Is it worth to first replace standard streams with "standard printers", and then close original streams? This allows to log warnings from closing streams.

Fixed.

> 3. "standard printers" are used at startup and at shutdown. Can we reuse some code?

I looked at the code creating standard printer during Python startup: it's just a few lines and it doesn't handle the case when stdout/stderr is already open. I don't think that it's worth to reuse code.

Anyway, with my new patch, the code is much more complex to handle the case if stderr and/or __stderr__ is "closed" (is NULL, None, getting closed attribute raises an error, or closed attribute is false).

> 4. Daemons close standard streams and fileno(stdout) can return unrelevant value.

Fixed.
msg262774 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-01 21:12
+    if (!closed) {
+        PyObject *res = PyObject_CallMethod(file, "close", "");
+        PyErr_Clear();
+        Py_XDECREF(res);
+    }
+    if (!dunder_closed) {
+        PyObject *res = PyObject_CallMethod(dunder_file, "close", "");
+        PyErr_Clear();
+        Py_XDECREF(res);
+    }

Hum, since it's common to have sys.__stderr__ = sys.stderr, maybe it's worth to skip the second close if dunder_file == file?
msg263255 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-12 15:46
@Serhiy: Would you mind reviewing replace_stdio-2.patch?
msg263626 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-17 20:30
I meant that C files stderr and stdout can be closed. Or it's file descriptors can be closed. I think in these cases fileno() or PyFile_NewStdPrinter() will fail.

I'm trying to consider all possible cases. Standard streams can be:

* Left original.
* Set to None.
* Reopened with different encoding/errors/etc.
* Redirected to a file (/dev/null).
* Redirected to a socket.
* Redirected to inherited file descriptor (pipe) in a subprocess.
* Be a high level wrapper around RPC (IDLE subprocess).

If the stream was reopened with the same file descriptor and closefd=True, closing it invalidates just opened "standard printer".

I would replace stdout and stderr after PyImport_Cleanup(). But PyImport_Cleanup() cleans up the sys module! Thus we should do this inside PyImport_Cleanup().
msg299192 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-07-26 02:58
I'm not sure that it's correct to replace sys.stdout with a simple object which can use a different encoding. I prefer to just close this old issue (idea).
History
Date User Action Args
2017-07-26 02:58:29vstinnersetstatus: open -> closed
resolution: out of date
messages: + msg299192

stage: resolved
2016-04-17 20:30:05serhiy.storchakasetmessages: + msg263626
2016-04-12 15:46:17vstinnersetmessages: + msg263255
2016-04-01 21:12:57vstinnersetmessages: + msg262774
2016-04-01 21:08:02vstinnersetfiles: + replace_stdio-2.patch

messages: + msg262772
2016-03-25 21:31:40vstinnersetmessages: + msg262456
2016-03-25 20:54:35serhiy.storchakasetmessages: + msg262455
2016-03-25 13:13:21vstinnersetnosy: + serhiy.storchaka
2016-03-25 13:13:17vstinnercreate