Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Doc/library/syslog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ The module defines the following functions:
it wasn't called prior to the call to :func:`syslog`, deferring to the syslog
implementation to call ``openlog()``.

.. versionchanged:: 3.12
Subinterpreters are allowed to call only if the main interpreter already called :func:`openlog`.
If not, it will raise :exc:`RuntimeError`.
Comment thread
corona10 marked this conversation as resolved.
Outdated


.. function:: openlog([ident[, logoption[, facility]]])

Expand All @@ -60,6 +64,10 @@ The module defines the following functions:
In previous versions, keyword arguments were not allowed, and *ident* was
required.

.. versionchanged:: 3.12
Only the main interpreter is allowed to call. It will raise :exc:`RuntimeError`
if subinterpreters call :func:`openlog`.
Comment thread
corona10 marked this conversation as resolved.
Outdated


.. function:: closelog()

Expand All @@ -72,6 +80,10 @@ The module defines the following functions:

.. audit-event:: syslog.closelog "" syslog.closelog

.. versionchanged:: 3.12
Only the main interpreter is allowed to call. It will raise :exc:`RuntimeError`
if subinterpreters call :func:`closelog`.
Comment thread
corona10 marked this conversation as resolved.
Outdated


.. function:: setlogmask(maskpri)

Expand Down
7 changes: 7 additions & 0 deletions Doc/whatsnew/3.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,13 @@ sys
with contributions from Gregory P. Smith [Google] and Mark Shannon
in :gh:`96123`.)

syslog
------

* :func:`syslog.openlog` and :func:`syslog.closelog` are only available from the main interpreter not subinterpreter.
:func:`syslog.syslog` is only available to subinterpreters if :func:`syslog.openlog` was already called from the main interpreter.
(Contributed by Dong-hee Na in :gh:`99127`.)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I think that the "Porting to Python 3.12" is a better section for these changes. The "syslog" section is more for new features.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can syslog.syslog only be used when the main interpreter has called syslog.openlog? That's a restriction that is not present in the underlying library.

Copy link
Copy Markdown
Member Author

@corona10 corona10 Nov 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronaldoussoren cc @ericsnowcurrently
This might be the discussion part for this change, from the view of managing for open/close state, subinterpreter should not intervene in changing the state of open/close. And this is why syslog.openlog and syslog.closelog are not available to the subinterpreter side. so if the subinterpreter can call syslog.syslog() without involving the main interpreter, it means that subinterpreter intervenes in the state of open/close so I think that it should be prevented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling openlog (and closelog) is optional in syslog(3) with sensible defaults. There is no need to call openlog at all unless a program wants to change some of the default settings, and that could be done outside of the Python world.

For a use-case like embedding Python in a web server (like mod_python it is more likely that the embedding program does the syslog setup than that this is done in Python code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling openlog (and closelog) is optional in syslog(3) with sensible defaults. There is no need to call openlog at all unless a program wants to change some of the default settings, and that could be done outside of the Python world.

In summary for your suggestion might be:

  • syslog.syslog(): Allow for both the main interpreter and subinterpreters at any condition.
  • syslog.openlog()/closelog(): Allow only for the main interpreter

But if the subinterpreter calls syslog.syslog() after the main interpreter calls the syslog.closelog(), the subinterpreter will neutralize what the main interpreter did. Who will close the syslog.syslog() that is opened by subinterpreter?

cc @ericsnowcurrently

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two factors here:

  1. most process-global resources should be managed by the app
  2. we are storing a str object in a static variable (S_ident_o), which is tricky when multiple interpreters are involved

Regarding the first one, this is perhaps something that we need not be so strict about. In Python, "app" means the __main__ module, running in the main interpreter (starting in the main thread). This implies that the global resources modifed via syslog.openlog() and syslog.closelog() should only be managed in the main interpreter. This would be similar to how signal handlers are supported only in the main thread of the main interpreter. However, this could be an overly strict interpretation for syslog. It makes sense to me, but "consenting adults", etc.

Regarding the second factor, we have been (and still are) working hard to get interpreters isolated from one another, especially to avoid problems that arise when they step on each others' toes. [1] (Isolation also provides new implementation opportunities.) For example, an object created in one interpreter should never be modified (even just the refcount) by another interpreter. [2] In the case of syslog, the object in the static variable (set in syslog.openlog()) should be managed only by the interpreter that owns it. Here are ways to enforce this:

  • the interpreter that calls openlog() should be the only one allowed to call closelog() (simplest way is to restrict to the main interpreter)
  • if another interpreter calls closelog(), then it would call Py_AddPendingCall() to have the owning interpreter decref the string

Obviously we went with the simplest form of the first one.

On top of the existing isolation concerns, there are additional ones that become more complicated under a per-interpreter GIL due to thread-safety. These are a non-issue if we restrict to a per-interpreter GIL.

[1] Such problems would be amplified by a per-interpreter GIL (see PEP 684, which has not been accepted yet), but they still exist even now.
[2] Except for a small set of objects managed by the process-global runtime.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, if the concern is backward compatibility, we can easily apply the restriction only if the subinterpreter was created via Py_NewInterpreter() (or, under a per-interpreter GIL, is otherwise sharing the GIL with the main interpreter).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling openlog (and closelog) is optional in syslog(3) with sensible defaults. There is no need to call openlog at all unless a program wants to change some of the default settings, and that could be done outside of the Python world.

For a use-case like embedding Python in a web server (like mod_python it is more likely that the embedding program does the syslog setup than that this is done in Python code.

FWIW, both concerns are valid, but pre-date this PR.



Optimizations
=============
Expand Down
64 changes: 64 additions & 0 deletions Lib/test/test_syslog.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import threading
import time
import unittest
from textwrap import dedent

# XXX(nnorwitz): This test sucks. I don't know of a platform independent way
# to verify that the messages were really logged.
Expand Down Expand Up @@ -78,6 +79,69 @@ def logger():
finally:
sys.setswitchinterval(orig_si)

def test_subinterpreter_syslog(self):
# syslog.syslog() is not allowed in subinterpreters, but only if
# syslog.openlog() hasn't been called in the main interpreter yet.
with self.subTest('before openlog()'):
code = dedent('''
import syslog
caught_error = False
try:
syslog.syslog('foo')
except RuntimeError:
caught_error = True
assert(caught_error)
''')
res = support.run_in_subinterp(code)
self.assertEqual(res, 0)

syslog.openlog()
try:
with self.subTest('after openlog()'):
code = dedent('''
import syslog
syslog.syslog('foo')
''')
res = support.run_in_subinterp(code)
self.assertEqual(res, 0)
finally:
syslog.closelog()

def test_subinterpreter_openlog(self):
try:
code = dedent('''
import syslog
caught_error = False
try:
syslog.openlog()
except RuntimeError:
caught_error = True

assert(caught_error)
''')
res = support.run_in_subinterp(code)
self.assertEqual(res, 0)
finally:
syslog.closelog()

def test_subinterpreter_closelog(self):
syslog.openlog('python')
try:
code = dedent('''
import syslog
caught_error = False
try:
syslog.closelog()
except RuntimeError:
caught_error = True

assert(caught_error)
''')
res = support.run_in_subinterp(code)
self.assertEqual(res, 0)
finally:
syslog.closelog()


if __name__ == "__main__":
unittest.main()
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow some features of :mod:`syslog` to the main interpreter only. Patch by Dong-hee Na.
29 changes: 27 additions & 2 deletions Modules/syslogmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,16 @@ module syslog

#include "clinic/syslogmodule.c.h"

/* only one instance, only one syslog, so globals should be ok */
static PyObject *S_ident_o = NULL; /* identifier, held by openlog() */
/* only one instance, only one syslog, so globals should be ok,
* these fields are writable from the main interpreter only. */
static PyObject *S_ident_o = NULL; // identifier, held by openlog()
static char S_log_open = 0;

static inline int
is_main_interpreter(void)
{
Comment thread
corona10 marked this conversation as resolved.
return (PyInterpreterState_Get() == PyInterpreterState_Main());
}

static PyObject *
syslog_get_argv(void)
Expand Down Expand Up @@ -135,6 +141,13 @@ syslog_openlog_impl(PyObject *module, PyObject *ident, long logopt,
long facility)
/*[clinic end generated code: output=5476c12829b6eb75 input=8a987a96a586eee7]*/
{
// Since the sys.openlog changes the process level state of syslog library,
// this operation is only allowed for the main interpreter.
if (!is_main_interpreter()) {
PyErr_SetString(PyExc_RuntimeError, "subinterpreter can't use syslog.openlog()");
return NULL;
}

const char *ident_str = NULL;

if (ident) {
Expand Down Expand Up @@ -195,6 +208,11 @@ syslog_syslog_impl(PyObject *module, int group_left_1, int priority,

/* if log is not opened, open it now */
if (!S_log_open) {
if (!is_main_interpreter()) {
PyErr_SetString(PyExc_RuntimeError, "subinterpreter can't use syslog.syslog() "
"until the syslog is opened by the main interpreter");
return NULL;
}
PyObject *openlog_ret = syslog_openlog_impl(module, NULL, 0, LOG_USER);
if (openlog_ret == NULL) {
return NULL;
Expand Down Expand Up @@ -230,6 +248,13 @@ static PyObject *
syslog_closelog_impl(PyObject *module)
/*[clinic end generated code: output=97890a80a24b1b84 input=fb77a54d447acf07]*/
{
// Since the sys.closelog changes the process level state of syslog library,
// this operation is only allowed for the main interpreter.
if (!is_main_interpreter()) {
PyErr_SetString(PyExc_RuntimeError, "sunbinterpreter can't use syslog.closelog()");
return NULL;
}

if (PySys_Audit("syslog.closelog", NULL) < 0) {
return NULL;
}
Expand Down