elgiano
December 9, 2021, 12:52am
1
Hello everybody!
A new RFC has been opened here:
supercollider:main ← elgiano:rtalloc-exceptions
opened 12:36AM - 09 Dec 21 UTC
- Title: Catch RTAlloc exceptions to prevent leaky behavior
- Date proposed: 20… 21-12-10
- RFC PR: https://github.com/supercollider/rfcs/pull/18
# Summary
UGens don't catch RTAlloc exceptions (e.g. alloc failed), leading to interruption of the processing block, and repeated calls to UGens constructors, leading to memory leaks. Comments are requested about how to best fix this issue for all affected UGens.
# Motivation
Every UGen that needs to allocate memory in real-time does so through RTAlloc, which throws an exception if there is not enough available memory. Those exceptions are never caught before the audio backend does so, meaning that the processing block in which they occurs exits early. Consequences are:
- no UGens after (in the tree) the one that threw the exception can process audio
- if the exception is thrown in a UGen constructor, the whole synth that contains that UGen doesn't advance from "construction-phase". This means that that constructor (and all other UGens' who are before it in the node tree) will be called again and again at every processing block
- Memory leak: if any of the repeteadly called constructors allocates memory via RTAlloc, references to previously allocated memory by those same UGens are lost.
Relevant open issues: #782 #3563 #5634
# Specification
RTAlloc is used in 125 times in 25 files in `server/plugins` (and 802 times in 164 files in `sc3-plugins`). Wouldn't it be nice to agree on how to fix best it before anyone starts editing all these places? I so much would like to have this issue fixed that I volunteer to do tedious work myself :)
So far we have discussed the following implementation plan. Any other input or comment is most welcome! Extra discussed points are summarized in the next session.
1. **remove the throw from RTAlloc and return `NULL` instead**: this will guarantee the same behavior for all of `AllocPool::Alloc()` 'failed' exit paths, make it consistent with `malloc`'s behavior, and ensure no exceptions are thrown across plugin boundaries. Exceptions can be used internally by both scsynth and sclang, but they shouldn't reach plugins, and in some compiler-specific situations, they actually can't reach them anyway. See comments [991629021](https://github.com/supercollider/rfcs/pull/18#issuecomment-991629021) and [991646042](https://github.com/supercollider/rfcs/pull/18#issuecomment-991646042)
2. double check that `server` and `sclang` don't need to differentiate between NULL-returning and exception-throwing exit paths from RTAlloc. Put NULL checks in server and sclang, where the exception can be thrown again if needed. See comment [991665839](https://github.com/supercollider/rfcs/pull/18#issuecomment-991665839) for a list of places to patch in `sclang`
3. implement **macros** for safer RTAlloc/RTFree in plugins (and sc3-plugins). See [995978596](https://github.com/supercollider/rfcs/pull/18#issuecomment-995978596) and [996065830](https://github.com/supercollider/rfcs/pull/18#issuecomment-996065830)
4. for UGens which do multiple allocations, ensure **all member pointers are initialized to NULL** before the first allocation. See [996010459](https://github.com/supercollider/rfcs/pull/18#issuecomment-996010459)
5. patch **`scfft_create`** as well. See [991677750](https://github.com/supercollider/rfcs/pull/18#issuecomment-991677750)
6. figure out what to do with UGens calling RTAlloc in `next()`. This doesn't lead to looping constructors, but still interrupts the processing block on failure as long as the `exception bug` is not fixed. The main difference with allocating in Ctor is that `next()` will retry the allocation at every processing block, at it's debatable wether failing in `next` should just do `ClearUnitOutputs()` (thus allowing for retry at next block) or rather `SETCALC(*ClearUnitOutputs)` (thus "turning off" the UGen forever). They can probably use the same macro, doing SETCALC(ClearUnitOutputs) since they all perform a "delayed initialization" and so they can be "deactivated" if they fail it (no need to let them try over and over again)
7. Update **documentation** for devs to know how to use RTAlloc with new macros in the future
# Drawbacks
It is a delicate work to patch those places in sclang and scsynth, as they're really core places, like PyrLexer or SC_SequenceMessage. The tedious work on plugin is the least issue, we'll need mostly thorough reviews and testing for point 2.
# Other points from the discussion
**Should UGens hold memory when failing?**
@jamshark70 If constructors are not called multiple times, single or multiple memory allocations would not leak, but just hold memory for doing nothing until the destructor is called and frees it all (and we need to put all the NULL checks there as well, otherwise the server crashes when trying to free memory that was eventually not allocated because the constructor failed early). So the question would be: should UGens hold any memory when they `SETCALC(*ClearUnitOutputs)`?
Since directly calling Unit destructors on RTAlloc fails was not considered a good practice, the idea is to just let Units hold the memory and let their destructor be called as usually when their Graph is destroyed. See https://github.com/supercollider/rfcs/pull/18#issuecomment-996052135
**Single vs Multiple RTAllocations**
UGens that allocate only once don't leak memory, because they fail with no side-effect on AllocPool. When allocating a single block and then splitting with multiple pointers:
> Apart from easier memory management, you also get better data locality which might result in fewer cache misses.
**mDone**
It could also be discussed what an UGen's behavior should be when it fails at RTAlloc. Setting the `mDone` flag looks like a sensible choice, as it would allow users to decide on whether to free the enclosing synth or just letting it run with that failed UGen clearing its outputs.
Please visit the above link for discussion.
Summary: when RTAlloc throws an exception (e.g. alloc failed), no UGens currently catch it and it is caught by the audio backend object. Consequences are interruption of the current processing cycle at that point, repeated UGen constructor calls, loss of references to allocated memory… Since the issue affects lots of UGens it would be beneficial to agree on how to design the fix before applying it to so many places.
4 Likes
yaaassss fantastic @elgiano !!!
This RFC is entering its Final Comment Period!
In 14 days, discussion will end and the proposal will either be accepted, rejected or withdrawn. Please see https://github.com/supercollider/rfcs#discussing-an-rfc for information on how to participate in the discussion, and https://github.com/supercollider/rfcs#finalizing-an-rfc for information on what Final Comment Period means.
Summary so far: RTAlloc shouldn’t throw an exception at all but return NULL consistently for all failed exit paths. This requires some adjustments in sclang, scynths and the introduction of new macros for server plugins.
1 Like