Fixes SIGSEGV and improve thread safety of journal.Reader class #144
Conversation
…th ref-counter; avoid segfault by closing journal across threads (closes systemdgh-143)
…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
|
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. |
| matrix: | ||
| python: [ | ||
| "3.7", | ||
| "3.8", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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" |
There was a problem hiding this comment.
Might make sense to always install setuptools, so that all versions are tested with the same?
There was a problem hiding this comment.
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. |
| PyObject_HEAD | ||
| sd_journal *j; | ||
| unsigned closed; | ||
| unsigned ref_count; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
When one looks for a reason for a bug, e.g. browsing the history of a file, or using
git blame, orgit 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.
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:
Reader_wait(), increment ref-count, release the lock (GIL), entersd_journal_wait()sd_journal_wait(), acquiring the lock (GIL), decrement ref-count, not callingsd_journal_close()because ofref_count == 1, leaveReader_wait()wait()however thread B invokesclose()in-between ...Reader_wait(), increment ref-count, release the lock (GIL), entersd_journal_wait()Reader_close(), set closed flag, decrement ref-count, but don't close it because ofref_count > 0sd_journal_wait(), acquiring the lock (GIL), decrement ref-count, callingsd_journal_close()because ofref_count == 0, leaveReader_wait()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 andReader.wait()raises an error ([Errno 22] Invalid argument).Anyway, no segfault happens anymore and it is more or less thread-safe.
closes gh-143