-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
fix failing pragma journal_mode=off
execution and subsequent crash
#1567
Conversation
c12ad54
to
06d121d
Compare
06d121d
to
2f0f904
Compare
Another option for #2 is to ignore '*-journal' files, though that also feels a bit janky. Can you show the output of |
I don't understand when SQLite accepts the setting change and when it doesn't. I sabotaged the test suite like this: diff --git a/coverage/sqldata.py b/coverage/sqldata.py
index 3e47b4d8..5c4690e6 100644
--- a/coverage/sqldata.py
+++ b/coverage/sqldata.py
@@ -1132,7 +1132,10 @@ def _connect(self) -> None:
# This pragma makes writing faster. It disables rollbacks, but we never need them.
# PyPy needs the .close() calls here, or sqlite gets twisted up:
# https://bitbucket.org/pypy/pypy/issues/2872/default-isolation-mode-is-different-on
+ jmode0 = self.execute_one("pragma journal_mode")
self.execute_void("pragma journal_mode=off")
+ jmode1 = self.execute_one("pragma journal_mode")
+ raise Exception(f"{sys.version.split()[0]}: {jmode0!r} -> {jmode1!r}")
if self.execute_one("pragma journal_mode") != "off":
# Some instances of Sqlite refuse to disable journal mode.
# Switching to memory mode prevents journal files from being written, Then ran it locally on my Mac, and also in GitHub Actions. These were the results:
Looking at the settings between the local versions that changed to "off" and those that kept "delete", they are the same:
|
Yeah, my original local quick-and-dirty fix was exactly that. I added a simple condition in diff --git a/coverage/data.py b/coverage/data.py
index abe6b510..7ab9fdcd 100644
--- a/coverage/data.py
+++ b/coverage/data.py
@@ -76,7 +76,7 @@ def combinable_files(data_file: str, data_paths: Optional[Iterable[str]] = None)
data_paths = data_paths or [data_dir]
files_to_combine = []
for p in data_paths:
- if os.path.isfile(p):
+ if os.path.isfile(p) and not p.endswith('.journal'):
files_to_combine.append(os.path.abspath(p))
elif os.path.isdir(p):
pattern = glob.escape(os.path.join(os.path.abspath(p), local)) +".*" That may or may not be what I actually tried; I'm just recreating it from memory. In any case, this felt too crude because I think if a journal file exists, that means that its paired sqlite db file might not actually have the change committed and may be slightly out of date, which means when coverage reads its contents, it may be incorrect or invalid. That's why I pursued the heavier-handed approach of checking
Sure, I'll switch over to that system shortly and include the output somewhere. |
34a8df9
to
5d25343
Compare
I switched to one of my problematic pyenv-installed Python installations, created a new venv, and confirmed
Some general info about this Python which was originally installed using
And finally coverage's debug output:
My homebrew-installed Python builds appears to correctly support
Diffing the output, there are several compile option differences between pyenv's and brew's: --- /Users/brasmith/Desktop/coverage_pyenv_python.txt 2023-03-20 17:02:42
+++ /Users/brasmith/Desktop/coverage_brew_python.txt 2023-03-20 17:07:11
@@ -1,6 +1,6 @@
-- sys -------------------------------------------------------
coverage_version: 7.2.2
- coverage_module: /Users/brasmith/projects/test_journal_mode/venv/lib/python3.10/site-packages/coverage/__init__.py
+ coverage_module: /Users/brasmith/projects/test_journal_mode/venv-brew/lib/python3.10/site-packages/coverage/__init__.py
tracer: -none-
CTracer: available
plugins.file_tracers: -none-
@@ -14,44 +14,37 @@
config_file: None
config_contents: -none-
data_file: -none-
- python: 3.10.5 (main, Jun 17 2022, 16:21:05) [Clang 13.1.6 (clang-1316.0.21.2.3)]
+ python: 3.10.10 (main, Feb 16 2023, 02:55:02) [Clang 14.0.0 (clang-1400.0.29.202)]
platform: macOS-13.2.1-x86_64-i386-64bit
implementation: CPython
- executable: /Users/brasmith/projects/test_journal_mode/venv/bin/python
+ executable: /Users/brasmith/projects/test_journal_mode/venv-brew/bin/python
def_encoding: utf-8
fs_encoding: utf-8
- pid: 41054
+ pid: 41353
cwd: /Users/brasmith/projects/test_journal_mode
- path: /Users/brasmith/projects/test_journal_mode/venv/bin
- /Users/brasmith/.pyenv/versions/3.10.5/lib/python310.zip
- /Users/brasmith/.pyenv/versions/3.10.5/lib/python3.10
- /Users/brasmith/.pyenv/versions/3.10.5/lib/python3.10/lib-dynload
- /Users/brasmith/projects/test_journal_mode/venv/lib/python3.10/site-packages
+ path: /Users/brasmith/projects/test_journal_mode/venv-brew/bin
+ /usr/local/Cellar/python@3.10/3.10.10_1/Frameworks/Python.framework/Versions/3.10/lib/python310.zip
+ /usr/local/Cellar/python@3.10/3.10.10_1/Frameworks/Python.framework/Versions/3.10/lib/python3.10
+ /usr/local/Cellar/python@3.10/3.10.10_1/Frameworks/Python.framework/Versions/3.10/lib/python3.10/lib-dynload
+ /Users/brasmith/projects/test_journal_mode/venv-brew/lib/python3.10/site-packages
environment: HOME = /Users/brasmith
PYENV_ROOT = /Users/brasmith/.pyenv
- command_line: /Users/brasmith/projects/test_journal_mode/venv/bin/coverage debug sys
- sqlite3_sqlite_version: 3.39.5
+ command_line: /Users/brasmith/projects/test_journal_mode/venv-brew/bin/coverage debug sys
+ sqlite3_sqlite_version: 3.40.1
sqlite3_temp_store: 0
- sqlite3_compile_options: ATOMIC_INTRINSICS=1, BUG_COMPATIBLE_20160819, CCCRYPT256,
- COMPILER=clang-14.0.0, DEFAULT_AUTOVACUUM, DEFAULT_CACHE_SIZE=2000,
- DEFAULT_CKPTFULLFSYNC, DEFAULT_FILE_FORMAT=4,
- DEFAULT_JOURNAL_SIZE_LIMIT=32768, DEFAULT_LOOKASIDE=1200,102,
- DEFAULT_MEMSTATUS=0, DEFAULT_MMAP_SIZE=0, DEFAULT_PAGE_SIZE=4096,
+ sqlite3_compile_options: ATOMIC_INTRINSICS=1, COMPILER=clang-14.0.0, DEFAULT_AUTOVACUUM,
+ DEFAULT_CACHE_SIZE=-2000, DEFAULT_FILE_FORMAT=4,
+ DEFAULT_JOURNAL_SIZE_LIMIT=-1, DEFAULT_MMAP_SIZE=0, DEFAULT_PAGE_SIZE=4096,
DEFAULT_PCACHE_INITSZ=20, DEFAULT_RECURSIVE_TRIGGERS,
DEFAULT_SECTOR_SIZE=4096, DEFAULT_SYNCHRONOUS=2,
- DEFAULT_WAL_AUTOCHECKPOINT=1000, DEFAULT_WAL_SYNCHRONOUS=1,
- DEFAULT_WORKER_THREADS=0, ENABLE_API_ARMOR, ENABLE_BYTECODE_VTAB,
- ENABLE_COLUMN_METADATA, ENABLE_DBSTAT_VTAB, ENABLE_FTS3,
- ENABLE_FTS3_PARENTHESIS, ENABLE_FTS3_TOKENIZER, ENABLE_FTS4, ENABLE_FTS5,
- ENABLE_LOCKING_STYLE=1, ENABLE_NORMALIZE, ENABLE_PREUPDATE_HOOK,
- ENABLE_RTREE, ENABLE_SESSION, ENABLE_SNAPSHOT, ENABLE_SQLLOG,
- ENABLE_STMT_SCANSTATUS, ENABLE_UNKNOWN_SQL_FUNCTION,
- ENABLE_UPDATE_DELETE_LIMIT, HAS_CODEC_RESTRICTED, HAVE_ISNAN,
+ DEFAULT_WAL_AUTOCHECKPOINT=1000, DEFAULT_WAL_SYNCHRONOUS=2,
+ DEFAULT_WORKER_THREADS=0, ENABLE_COLUMN_METADATA, ENABLE_FTS3,
+ ENABLE_FTS3_PARENTHESIS, ENABLE_FTS4, ENABLE_FTS5, ENABLE_GEOPOLY,
+ ENABLE_MATH_FUNCTIONS, ENABLE_PREUPDATE_HOOK, ENABLE_RTREE, ENABLE_SESSION,
MALLOC_SOFT_LIMIT=1024, MAX_ATTACHED=10, MAX_COLUMN=2000,
MAX_COMPOUND_SELECT=500, MAX_DEFAULT_PAGE_SIZE=8192, MAX_EXPR_DEPTH=1000,
- MAX_FUNCTION_ARG=127, MAX_LENGTH=2147483645, MAX_LIKE_PATTERN_LENGTH=50000,
- MAX_MMAP_SIZE=1073741824, MAX_PAGE_COUNT=1073741823, MAX_PAGE_SIZE=65536,
+ MAX_FUNCTION_ARG=127, MAX_LENGTH=1000000000, MAX_LIKE_PATTERN_LENGTH=50000,
+ MAX_MMAP_SIZE=0x7fff0000, MAX_PAGE_COUNT=1073741823, MAX_PAGE_SIZE=65536,
MAX_SQL_LENGTH=1000000000, MAX_TRIGGER_DEPTH=1000,
- MAX_VARIABLE_NUMBER=500000, MAX_VDBE_OP=250000000, MAX_WORKER_THREADS=8,
- MUTEX_UNFAIR, OMIT_AUTORESET, OMIT_LOAD_EXTENSION, STMTJRNL_SPILL=131072,
- SYSTEM_MALLOC, TEMP_STORE=1, THREADSAFE=2, USE_URI
+ MAX_VARIABLE_NUMBER=250000, MAX_VDBE_OP=250000000, MAX_WORKER_THREADS=8,
+ MUTEX_PTHREADS, SYSTEM_MALLOC, TEMP_STORE=1, THREADSAFE=1
|
5d25343
to
382babd
Compare
#1605 reports the same issue with journal files interfering with combining. |
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
382babd
to
72a12ee
Compare
I recently encountered an issue that would sometimes cause coverage to crash despite no changes to the code it was testing/running against. Apparently some builds of Python+SQLite refuse to apply
pragma journal_mode=off
(they allow it to "execute" but actually silently ignore it), and that can results in short-lived*-journal
files appearing on the filesystem since the defaultpragma journal_mode=delete
is still in effect. When coverage runs in multi/parallel mode and attempts to combine results from its separate files, sometimes (presumably depending on OS/filesystem sync timing) some of the*-journal
files may still briefly exist, and this can cause coverage to crash.Real-world example of coverage crashing at the end of testing a project I was recently working on:
It was difficult to isolate a reproducible example with coverage itself running against another project, but I was able to at least verify independently that, yes, some builds of Python+SQLite refuse or ignore
pragma journal_mode=off
. See: https://github.com/infinitewarp/test-python-sqlite-journal_mode. The TLDR is that the default macOS 13.2.1 build and pyenv-installed builds fail, but homebrew-installed builds and Linux builds inside various Docker images work as expected.Since I would like coverage to work reliably in my pyenv-installed environments, I'm proposing two changes with this PR, but I'm happy to drop one in favor of the other or to consider alternate implementations:
pragma journal_mode=off
actually succeeded. If it didn't, executepragma journal_mode=memory
so the filesystem should never be dirtied with*-journal
files. Based on my local testing and understanding of the SQLite documentation, thememory
mode should still be available even whenoff
is not. This might impact memory use or performance, but I suspect the effect would be negligible and a worthwhile tradeoff for stability.combine_parallel_data
with atry
, and handle theFileNotFoundError
exception. This would help in the case where the-journal
file exists whencombinable_files
is called but has been deleted by the timecombine_parallel_data
tries to read it. Weird as it sounds, that does appear to happen sometimes. This handling shouldn't be necessary if 1 succeeded in settingpragma journal_mode=memory
, and I considered dropping this part from the PR, but I thought it might not hurt to have some extra checking there.Please let me know what you think. 🙂 This problem took a while to figure out since the behavior was so inconsistent between runs and environments, and I'm happy to revise this PR or consider alternate ideas if you have them. I feel like neither 1 nor 2 is ideal, but given the fact that some Python+SQLite builds are misbehaving, I couldn't think of a better solution.