-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Valgrind Memory Leak Checking #8954
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
base: main
Are you sure you want to change the base?
Conversation
* Free the output buffer on webp encode error
* If setimage errors out, the tiff client state was not freed.
* Return after setting the error for advanced features without libraqm. Not returning here leads to an alloc that's never freed.
@wiredfool We can trigger a workflow manually or run daily independent of PRs. |
@@ -99,6 +99,13 @@ valgrind: | |||
--log-file=/tmp/valgrind-output \ | |||
python3 -m pytest --no-memcheck -vv --valgrind --valgrind-log=/tmp/valgrind-output | |||
|
|||
.PHONY: valgrind-leak | |||
valgrind-leak: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valgrind-leak: | |
valgrind-leak: | |
python3 -c "import pytest_valgrind" > /dev/null 2>&1 || python3 -m pip install pytest-valgrind |
Install pytest-valgrind if it is missing, like the 'valgrind' target does
src/libImaging/Arrow.c
Outdated
@@ -37,6 +37,10 @@ ReleaseExportedSchema(struct ArrowSchema *array) { | |||
child->release = NULL; | |||
} | |||
// UNDONE -- should I be releasing the children? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this UNDONE comment resolved now?
The question then is how to we prevent this from being run as a scheduled action and subsequently ignored? The tension is that this is by far the most valuable when a PR is in flight, because then it's obvious when you've gone from current (xfail) to fail, but the runtime is likely 2-3x worse than our longest other test run, which is already too long. (Even running single tests with valgrind take ~ 1 min, because all the pytest setup infra runs under valgrind as well) |
In that case, long PR it is! We typically have PRs queued for days, or longer, before merging. |
On My Machine: That's vs 87 minutes for ordinary valgrind on this pr in the GHA testing. Unfortunately, we're single threaded for valgrind tests. |
Ok, We'll see how long this takes: https://github.com/python-pillow/Pillow/actions/runs/15054197221/job/42316085246?pr=8954 I'm only running this on the pull request, not the push. I'm expecting 5 hours. This is injecting the test script in the Pillow repo, instead of building a new copy of the valgrind image. At some point we can move that over, but I'd like to be tuning the supressions without having to build a new valgrind image, so it's probably better to leave it here for now. |
Changes proposed in this pull request:
To Do:
Note -- this is built on the Arrow memory leak check PR #8953.