fix: mark tests expecting malloc to fail as opt-in#15097
Conversation
ffb4801 to
37f4e7b
Compare
Those tests might overload machines with huge amount of RAM or with overcommit enabled, because of that - one test was already disabled on darwin, which enables overcommit by default. Ref: Comments in pyca#14782 Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
37f4e7b to
2a3bb69
Compare
|
Is there a particular place this is causing a problem, or just in theory? Tests should (probably must ) be on by default to be effective, so I don't want to disable things by default without an exceptionally good reason. |
This test was already disabled on darwin, and it fails on any linux machine with overcommit enabled ( I have noticed this problem because I was getting OOM during this package build in NixOS, but this may happen regardless of the distribution, the original PR has a comment telling about the OOM on Gentoo machine. This test should not be run by default, because by spec, malloc only fails in case of system is unable to allocate memory, but with overcommit memory allocation always succeeds, regardless of the size. |
|
I'd be happy to consider a PR that checks the sysctl for overcommit and skips in those cases, but right now this PR makes these tests less useless for the developers of this software. |
|
I'm not sure if checking sysctl is a good idea here, it might not be available in docker/with selinux/other sandboxing, while explicit Better behavior in my opinion would be trying to allocate 4TiB block, and then skipping |
08144da to
490e521
Compare
|
Implemented, but have not tested yet the second solution I proposed (probe for overcommit, skip tests if environment allows overcommits) Just to be clear - OOM happens not because of malloc, but because it actually uses the allocated memory, so this probe won't cause the same problem |
Signed-off-by: Yaroslav Bolyukin <iam@lach.pw>
490e521 to
459c46b
Compare
| # has more RAM, or overcommits. | ||
| try: | ||
| m = mmap.mmap(-1, (2**32 - 1) * 1024) | ||
| except (OSError, OverflowError): |
There was a problem hiding this comment.
CI was failing on armv7l with OverflowError here, because 4TiB can't be converted to ssize_t (2GB) on 32bit platforms
I don't think it makes sense to check for overcommit in such cases, all the malloc failure tests allocate much more that 2GB already and will never fail on overcommit enabled on 32bit platform
|
Tested in overcommiting environment - the test gets properly skipped, and I don't see it being skipped in CI, so everything seems to be fine |
|
Also found out two downstreams were already disabling one of the failing tests, exactly because of overcommits: With the new pytest mark added, new such tests would no longer be disruptive for maintainers |
Those tests might overload machines with huge amount of RAM or with overcommit enabled, because of that - one test was already disabled on darwin, which enables overcommit by default.
Because of the potentially destructive overhead, I believe they should be opt-in.
Ref: Read the comments in #14782