classification
Title: Introduce option to force the interpreter to exit upon MemoryErrors
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: barry, benjamin.peterson, christian.heimes, ctheune, ebfe, gvanrossum, hynek, jcea, pitrou, vstinner
Priority: normal Keywords: patch

Created on 2012-11-01 13:15 by ctheune, last changed 2019-10-23 16:27 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
issue16381.diff ctheune, 2012-11-01 13:48 review
fatalmalloc.c vstinner, 2013-10-10 23:11
setup.py vstinner, 2013-10-10 23:11
Repositories containing patches
https://bitbucket.org/ctheune/cpython
Messages (21)
msg174413 - (view) Author: Christian Theune (ctheune) * Date: 2012-11-01 13:15
I run long-running server processes (web apps, etc) a lot and I keep encountering the situation that many applications will not properly deal with MemoryError exceptions but end up in an unusable state.

From an operational perspective I wish the process in this case would just fail and exit.

I talked to Guido about this general idea at EuroPython2012 and he encouraged me to look into this. 

Here's a patch:
https://bitbucket.org/ctheune/cpython/changeset/323bb572344d46df669d3dbec4431cf6720fc5b4

I think it does what I want it to do, but a) my C knowledge is really bad and b) I'm not sure whether this is the right approach anyway.

I'd appreciate feedback and possibly inclusion in the core.
msg174415 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-01 13:26
Your proposal sounds like a very good idea. IMHO it should be discussed on the python-ideas or python-dev mailing list before it gets integrated into 3.4. Embrace yourself for some serious bike shedding! :)

By the way your patch contains several changes that aren't related to your proposal. Can you clean up your patch, please? It makes code review much easier.
msg174419 - (view) Author: Christian Theune (ctheune) * Date: 2012-11-01 13:48
Grr. Sorry. The automatic patch extraction went wrong and I didn't notice. Here's a manual try.
msg174423 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-01 15:35
Thanks!

Py_FatalError() might be too drastic for the task. It calls abort() which kills the process with SIGABRT. The function closes and flushes all stream but no additional cleanup code is executed. This might be bad for resources like shared memories, named semaphores or database connections. On Windows abort() causes a pop up window with a crash report, too.

Would exit(3) or _exit(2) work here?
msg174424 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-11-01 15:36
I think the fatal erroring should be done in PyErr_NoMemory.
msg174495 - (view) Author: Christian Theune (ctheune) * Date: 2012-11-02 08:47
I pondered PyErr_NoMemory as well. However, I noticed not all locations in Python use PyErr_NoMemory to raise a MemoryError, and I'm also afraid that external libraries will have the same problem.

Can you explain why you consider PyErr_NoMemory to be the better option? (I can think of performance but I wouldn't want to trade it in against reliability.)
msg174510 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-11-02 12:55
Where it is now means raising a MemoryError in Python code will cause the fatal error. I don't know if that's a good idea.
msg174512 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-11-02 13:46
There used to be some places in the C code that raise MemoryError to
indicate that some parameter indicating a desired result size is out
of range, i.e. before even trying to allocate anything. I can't
confirm any, but these should probably be replaced with another
exception. (I thought string times integer was one such place, but it
raises OverflowError now.) If there are any left those should probably
be replaced with OverflowError.
msg174513 - (view) Author: Lukas Lueg (ebfe) Date: 2012-11-02 13:54
I have to say this feels like spooky action at a distance.

Wouldnt it be less intrusive - while achieving the same result - to make MemoryError uncatchable if the flag is set?
msg174515 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-02 14:04
MemoryError can be raised under two different circumstances that should be handled differently. Either the program tries to allocate a rather large chunk of memory for e.g. a string with a couple of hundred KB and more. Or Python can't malloc() even small amounts of memory for its most basic operations like raising an exception object. That's why PyErr_NoMemory() exists and why it uses a pre-allocated MemoryError object.

When a program can't allocate memory for an image it usually has enough memory left to do proper error reporting and shut down. However when even "x = 2 * 200" fails with a memory error then proper shutdown will most likely fail and hang up the process, too.

Too bad that PyErr_NoMemory() is used for both scenarios and isn't aware how much memory was requested.
msg174517 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-11-02 14:27
Well, you can fix that, right? Just add a new function with a better
signature and use that for one of the two scenarios.

(I think  third scenario might be when realloc fails -- IIRC it
doesn't guarantee that the original pointer is still valid either?)
msg174524 - (view) Author: Lukas Lueg (ebfe) Date: 2012-11-02 14:48
In any strategy only a heuristic could be used in order to decide wether or not it's safe to raise MemoryError. How exactly is memory pressure expected for x=[2]*200 but not for x=2*200 ?

I don't think a new function could ultimatly achieve it's goal. If MemoryError occurs, all bets are off. Shutting down as fast as possible is the best we could do, if requested.
msg174527 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-02 14:50
Of course it can be fixed. But it's going to be a long and tedious piece of work. PyErr_NoMemory() is called 314 times. MemoryError is raised about 40 times in C code.

I'm not sure what your question about realloc() really is. Are you unsure how a a failing realloc() calls must be resolved?

Code like

  ptr = realloc(ptr, size);

is usually a memory leak as it leaks the previously allocated memory in ptr. In the error case the block in ptr is left untouched and is still valid. It must be freed manually or can be used further and freed later.

  if((ptr2 == realloc(ptr, size)) == NULL) {
    free(ptr);
  }
  else {
    ptr = ptr2;
  }
msg174532 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-11-02 14:59
We can create some kind of heuristic when the memory error handler knows the size of the new block and the size difference on realloc(). It could be an application specific threshold, too.
msg174533 - (view) Author: Lukas Lueg (ebfe) Date: 2012-11-02 15:10
The heuristic basically has to decide if memory pressure is so high that it's not save to return to the interpreter. Even if there is a chosen value (e.g. failed allocation attempts below 1mb are considered fatal), there can always be another OS-thread in the interpreter process that eats away exactly that memory while we are returning MemoryError - the program might still hang.
FWICS all MemoryErrors are to be considered fatal or none of them
msg184397 - (view) Author: Christian Theune (ctheune) * Date: 2013-03-17 20:35
I feel unsure how to help this move along. 

I agree that making it possible for applications to carefully work with MemoryErrors is a good idea. I don't think heuristics to determine which situation we are in will solve this but make it more spooky. (This is a semantic issue anyways).

My goal is to use the flag to make it easier for operators to deal with applications that don't get it right. This isn't to defend bad programming but to support temporary relief.
msg184482 - (view) Author: Lukas Lueg (ebfe) Date: 2013-03-18 17:57
Another proposal: Add a new BaseClass that, if inherited from, causes an exception to be uncatchable (e.g. class HardMemoryError(MemoryError, UncatchableException)).
msg199117 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-06 21:06
If we want to get more sophisticated, I would suggest a two-pronged approach:
- first call a user-defined oom function
- if calling the user-defined oom function raises MemoryError, dump a fatal error
msg199118 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-06 21:27
See also issue #1195571: simple callback system for Py_FatalError. It is somehow related.
msg199428 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-10-10 23:11
This issue is maybe a new usecase of the PEP 445.

Try attached Python module fatalmalloc.c, use attached setup.py script to build it.

$ python3.4 -c 'import fatalmalloc; x="x"*(50*1024*1024); print(len(x))'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
MemoryError

$ python3.4 -c 'import fatalmalloc; fatalmalloc.enable(); x="x"*(50*1024*1024)'
$ echo $?
1
msg355238 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-23 16:27
It is possible to install a custom hook on memory allocators using the PEP 445: see for example attached fatalmalloc.c.

I don't see much traction to get this feature since 2013, I reject the feature request.
History
Date User Action Args
2019-10-23 16:27:42vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg355238

stage: patch review -> resolved
2013-10-10 23:11:12vstinnersetfiles: + setup.py
2013-10-10 23:11:03vstinnersetfiles: + fatalmalloc.c

messages: + msg199428
2013-10-06 21:27:36vstinnersetmessages: + msg199118
2013-10-06 21:06:13pitrousetnosy: + pitrou
messages: + msg199117
2013-10-06 21:01:42vstinnersetnosy: + vstinner
2013-03-18 17:57:01ebfesetmessages: + msg184482
2013-03-17 22:45:30ctheunesetnosy: + barry
2013-03-17 20:35:49ctheunesetmessages: + msg184397
2012-11-06 15:28:10jceasetnosy: + jcea
2012-11-02 15:10:24ebfesetmessages: + msg174533
2012-11-02 14:59:43christian.heimessetmessages: + msg174532
2012-11-02 14:50:30christian.heimessetmessages: + msg174527
2012-11-02 14:48:14ebfesetmessages: + msg174524
2012-11-02 14:27:14gvanrossumsetmessages: + msg174517
2012-11-02 14:04:22christian.heimessetmessages: + msg174515
2012-11-02 13:54:15ebfesetnosy: + ebfe
messages: + msg174513
2012-11-02 13:46:26gvanrossumsetmessages: + msg174512
2012-11-02 12:55:53benjamin.petersonsetmessages: + msg174510
2012-11-02 08:47:23ctheunesetmessages: + msg174495
2012-11-02 06:58:19hyneksetnosy: + hynek
2012-11-01 15:36:48benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg174424
2012-11-01 15:35:21christian.heimessetmessages: + msg174423
2012-11-01 13:52:06ctheunesetfiles: - 9430a5c65114.diff
2012-11-01 13:48:18ctheunesetfiles: + issue16381.diff

messages: + msg174419
2012-11-01 13:26:09christian.heimessetnosy: + christian.heimes

messages: + msg174415
stage: patch review
2012-11-01 13:15:58ctheunesetfiles: + 9430a5c65114.diff
keywords: + patch
2012-11-01 13:15:04ctheunecreate