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)
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. |
| concurrency: | ||
| group: ${{ github.workflow }}-${{ matrix.python }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
| strategy: | ||
| fail-fast: false | ||
| 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.
| @@ -43,6 +44,9 @@ jobs: | |||
| run: | | |||
| 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.
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