Fixes SIGSEGV and improve thread safety of journal.Reader class by sebres · Pull Request #144 · systemd/python-systemd · GitHub

This PR fixes threaded issues of journal.Reader class and improves its thread-safety, using simple protection with ref-counter;
It would avoid segfault by closing journal across threads.

The algorithm is simple, here short explanation illustrating the approach on an example of #143:

TH	Action	ref_count	j_handle
...	Reader created and has initial ref-count 1	1	valid
A	Enter Reader_wait(), increment ref-count, release the lock (GIL), enter sd_journal_wait()	2	valid
A	Leave sd_journal_wait(), acquiring the lock (GIL), decrement ref-count, not calling sd_journal_close() because of ref_count == 1, leave Reader_wait()	1	valid
-	... now thread A repeats the wait() however thread B invokes close() in-between ...	-	-
A	Enter Reader_wait(), increment ref-count, release the lock (GIL), enter sd_journal_wait()	2	valid
B	Enter Reader_close(), set closed flag, decrement ref-count, but don't close it because of ref_count > 0	1	valid
A	Leave sd_journal_wait(), acquiring the lock (GIL), decrement ref-count, calling sd_journal_close() because of ref_count == 0, leave Reader_wait()	0	NULL

If thread B manages to close the handle before A enters wait mode, it is also safe, because then sd_journal_wait() would generate EINVARG and Reader.wait() raises an error ([Errno 22] Invalid argument).

Anyway, no segfault happens anymore and it is more or less thread-safe.

closes gh-143

…th ref-counter; avoid segfault by closing journal across threads (closes systemdgh-143)

…ids unexpected states on repeated Reader.close)

…ages, etc)

7c99a39 seems to fix the GHA flows now (completely different issue), so can be cherry-picked to main branch as it is (or I could make a separate PR if necessary).

Ping.
Is this repository alive?

That should probably be dropped as well, since it is EOL or in other words: with 3.7 dropped I see no reason not to drop 3.8

I dropped 3.7 only because of switch to ubuntu-latest, which doesn't have it.
I don't see any reason to drop 3.8. at least as long as it is compatible and running yet.
Anyway has nothing to do with the PR as is, I made the changes just in order to repair the GHA flow.

Might make sense to always install setuptools, so that all versions are tested with the same?

Dunno... IMHO, only 3.12+ missing it as package, but I am unsure the attempt to install with pip for previous versions would be always successful (and not fail due to conflict with preinstalled package).
I can imagine it could.

The PR #147 will help fix the GH Action headache for this PR.

The PR #147 will help fix the GH Action headache for this PR.

Hmm... There is basically no problem to fix "for this PR" - as you can see all GHA checks are passed.
Nothing about #147 (maybe it solves more, no time to review), just... actually everything work here in this PR as expected.

I think the thread-safety stuff is confused. The last commit seems useful, it might be worth submitting it as a separate PR.

Note we also have #148 which tries to update the build system.

In the future, please explain the reasoning behind a patch in the commit message. For something like thread safety, this is particularly important. Without that, the reader is left to guess.

I don't understand how this is supposed to help. This is a python object, and PyObject_HEAD above already embeds a reference counter. A second reference counter in the same struct is unexpected.

Oh, you did provide an explanation, but in the pull request ticket. I was looking at the patches and missed that. Sorry. (The explanation still belongs in the commit message, not on a pull request.)

I'll reply further in #143, because you make some good points there.

The explanation still belongs in the commit message, not on a pull request.

Well, matter of taste either (many repo-owners like to hold the commit messages as short as possible)...
But it can be added to the merge commit, can not it?
Or do you rather want that the PR branch gets rebased instead (with details in the commit)?

Well, matter of taste either (many repo-owners like to hold the commit messages as short as possible)...

Not this repo certainly. Frankly, I don't think that'd make any sense. One can always not look at the commit descriptions if they don't want to.

But it can be added to the merge commit, can not it?

This doesn't work properly. When one looks for a reason for a bug, e.g. browsing the history of a file, or using git blame, or git bisect, they'll see the commit. Obviously, with enough effort, one can find the commit message, and the discussion on github, or even the mailing list thread, but those are all inferior options.

Or do you rather want that the PR branch gets rebased instead (with details in the commit)?

In general, I'd like to see the PR rebased until it looks properly, per the usual open-source workflow. In this particuluar case, let's not rebase yet. I have more comments cooking.

When one looks for a reason for a bug, e.g. browsing the history of a file, or using git blame, or git bisect, they'll see the commit.

No, by default git blame as well as git bisect would show/use the merge commit firstly. Even blaming on github shows firstly the merge point and only with "Blame prior to change" you'd get the original commit.

But by merging of PR, GH also allows to squash everything to single commit instead of merge, and one can rewrite the message.

In this particuluar case, let's not rebase yet. I have more comments cooking.

OK, no problem.

No, by default git blame as well as git bisect would show/use the merge commit firstly.

I don't think so. For an arbitrary file I have here, git blame src/core/main.c|cut -d' ' -f1 |sort -u|xargs -n1 git log --pretty=oneline -1 doesn't show even one merge commit.

But by merging of PR, GH also allows to squash everything to single commit instead of merge, and one can rewrite the message.

Sure, but that's only useful for PRs that should be one commit. Here we have additional changes…

For an arbitrary file I have here, git blame ... doesn't show even one merge commit.

I think it is git blame --first-parent $file... Many UIs (and GH) doing it by default, so I assumed cmd-line would do that too.

sebres added 2 commits April 1, 2025 16:33

improve thread safety of journal.Reader class by simple protection wi…

245784c

…th ref-counter; avoid segfault by closing journal across threads (closes systemdgh-143)

amend: close and decrement count for initial reference only once (avo…

7fa91c8

…ids unexpected states on repeated Reader.close)

sebres force-pushed the gh-143-sf-fix branch 2 times, most recently from 5a7719b to 03eec9d Compare April 2, 2025 16:31

repair GHA-CI (update versions, pip install using --break-system-pack…

7c99a39

…ages, etc)

sebres force-pushed the gh-143-sf-fix branch from 03eec9d to 7c99a39 Compare April 2, 2025 17:06

behrmann reviewed Jun 4, 2025

View reviewed changes

keszybz reviewed Jul 1, 2025

View reviewed changes

remove noop, unneeded by dealloc

646b3b3

keszybz mentioned this pull request Oct 11, 2025

Modernize code and silence all warnings #160

Merged

                     matrix:
                       python: [
-                        "3.7",
                         "3.8",

                         sudo apt -y update
                         sudo apt -y install gcc libsystemd-dev
+                        if dpkg --compare-versions "${{ matrix.python }}" ge 3.12; then
+                          python -m pip install setuptools || echo "can't install setuptools"

                       PyObject_HEAD
                       sd_journal *j;
+                      unsigned closed;
+                      unsigned ref_count;

Fixes SIGSEGV and improve thread safety of journal.Reader class by sebres · Pull Request #144 · systemd/python-systemd · GitHub

Uh oh!

Fixes SIGSEGV and improve thread safety of journal.Reader class - #144

Fixes SIGSEGV and improve thread safety of journal.Reader class#144sebres wants to merge 4 commits intosystemd:mainsystemd/python-systemd:mainfrom sebres:gh-143-sf-fixsebres/python-systemd:gh-143-sf-fixCopy head branch name to clipboard

Conversation

sebres commented Apr 1, 2025

Uh oh!

sebres commented Apr 2, 2025

Uh oh!

sebres commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongquan commented Jun 28, 2025

Uh oh!

sebres commented Jun 30, 2025

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebres Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Fixes SIGSEGV and improve thread safety of journal.Reader class#144
sebres wants to merge 4 commits into
systemd:mainfrom
sebres:gh-143-sf-fix

sebres Oct 3, 2025 •
edited
Loading