Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port syslog module to use module state #99127

Open
corona10 opened this issue Nov 5, 2022 · 5 comments
Open

Port syslog module to use module state #99127

corona10 opened this issue Nov 5, 2022 · 5 comments
Assignees
Labels
expert-subinterpreters type-feature A feature request or enhancement

Comments

@corona10
Copy link
Member

corona10 commented Nov 5, 2022

While I check the @ericsnowcurrently 's checklist: https://github.com/ericsnowcurrently/multi-core-python/wiki/0-The-Plan
I found that the syslog module still uses the global variable for the following variables.

  • S_ident_o
  • S_log_open

Both variables are declared as global variables since the original author assumes that in only one instance, only one syslog will exist.(one process one interpreter)

So I think that it's okay to move them into the module state if we consider the per-interpreter model.

@corona10 corona10 added the type-feature A feature request or enhancement label Nov 5, 2022
@corona10 corona10 self-assigned this Nov 5, 2022
corona10 added a commit to corona10/cpython that referenced this issue Nov 5, 2022
corona10 added a commit to corona10/cpython that referenced this issue Nov 5, 2022
corona10 added a commit to corona10/cpython that referenced this issue Nov 5, 2022
@corona10
Copy link
Member Author

corona10 commented Nov 5, 2022

Hmm, I think that I miss understood about syslog, I close ths issue as invalid.

@corona10 corona10 closed this as completed Nov 5, 2022
@ronaldoussoren
Copy link
Contributor

ronaldoussoren commented Nov 5, 2022

I've added some review comments to the PR, it seems to be on right path with some minor issues because the syslog extension changes process global state in the syslog library. Those issues IMHO can be easily fixed.

@corona10 corona10 reopened this Nov 5, 2022
@corona10
Copy link
Member Author

corona10 commented Nov 5, 2022

I've added some review comments to the PR, it seems to be on right path with some minor issues because the syslog extension changes process global state in the syslog library. Those issues IMHO can be easily fixed.

Thanks I will take a look.

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Nov 7, 2022

Note that currently the syslog module already has the following characteristics:

  • an app that embeds CPython may call openlog() independently of any use of the syslog module, and the two may interfere with each other
  • two interpreters using the syslog module may interfere with each other in the same way

We can't do much about the embedding case because the posix syslog API doesn't provide a way to get the current configuration (if any) such that you could restore it.

For multiple interpreters, either we leave the current behavior or we try to make each interpreter independent. To do that, we have two options:

  • swap in each interpreter's configuration around each syslog.syslog() call
    • call openlog() before and closelog() after
    • call setlogmask() before and after
    • syslog.openlog() and syslog.setlogmask() would only store the values on the module state
  • do similar, but be smarter about the single-interpreter case and cases where swapping is not necessary (i.e. same syslog config)

The "smarter" approach would require that the syslog module keep process-global state, along with a lock to protect that state. That would require something similar to what is discussed in https://discuss.python.org/t/20668 (and https://discuss.python.org/t/20663).


For reference:

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Nov 7, 2022

FWIW, for an implementation of mutli-phase init for a module, I would expect it to either deal with the sort of weird behavior syslog would have, or disallow use in multiple interpreters (at least in problematic cases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expert-subinterpreters type-feature A feature request or enhancement
Projects
Status: No status
Development

No branches or pull requests

3 participants