Deal with RTAlloc exceptions in Graphs to prevent memory leaks

[elgiano]

• Title: Catch RTAlloc exceptions to prevent leaky behavior
• Date proposed: 2021-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 and 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 for a list of places to patch in sclang
3. implement macros for safer RTAlloc/RTFree in plugins (and sc3-plugins). See 995978596 and 996065830
4. for UGens which do multiple allocations, ensure all member pointers are initialized to NULL before the first allocation. See 996010459
5. patch scfft_create as well. See 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.

[Spacechild1]

RTAlloc, which throws an exception if there is not enough available memory.

I'm not sure. I just looked at SC_AllocPool.cpp and yes, they throw exceptions, but there are also code paths where it returns 0. In fact, we have always been told that RTAlloc and RTRealloc would return NULL on failure and that we always have to check the result. This is what many core plugins do. I think there was even an initiative to add more null-checks to sc3-plugins.

So first verify if a non-growable AllocPool can really throw an exception. You can write a test plugin that allocates RT memory in an infinite loop and breaks from the loop if the result is NULL.

UGens that use RTAlloc should be responsible for catching the exception.

No plugin interface method must ever throw an exception! Generally, C++ exceptions must not cross plugin boundaries because they are not compatible across compilers.

---

Generally, C++ exceptions only really make sense together with RAII (https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization), but scsynth is mostly written in an early "C with classes" style with manual memory management (which is kind of necessary because of the stateful allocators). Supernova is a different story, btw.

[jamshark70]

I'm not completely sure whether all of this will hold water, but...

RTAlloc-ing UGens could be divided into two categories:

A. UGens that perform exactly one RTAlloc (e.g., DelayX, CombX, AllpassX, LocalBuf) -- probably many/most of them.

B. UGens that perform two or more RTAllocs (e.g. PartConv, PV_BinScramble, ...).

Category A:

I would assume that a failed RTAlloc request would leave the allocator in the same state as before the request. (Is that a safe assumption?)

If this is the case, then A-type UGens would not need to release any memory upon failure. They should set the calculation function to clear unit outputs (to prevent wasting time with more requests that we already know will fail).

So these are simpler.

Category B:

Before calling Ctor(), there is no way to know whether the first, second, third etc. request will fail. So there might be some successful requests. In that case, the UGen needs to be responsible for releasing the successfully allocated blocks. If RTAlloc might throw an exception, then it seems a try/catch at the point of allocation would be mandatory. (There's no way for the caller to access the memory addresses to be released -- I think?)

(I noticed here that PV_BinScramble calls RTAlloc() in its next() function, not in the Ctor, btw.)

An alternate approach would be to replace multiple allocations with a single, larger allocation, and apportion out the pointers as needed. Actually PV_BinScramble_next() shows an example:

        unit->m_to = (int*)RTAlloc(unit->mWorld, numbins * 2 * sizeof(int));
        unit->m_from = unit->m_to + numbins;

... where two buffers with numbins ints are needed. The single RTAlloc call gets both of them at once, and then simply assigns a second pointer based on the known size.

Then it goes on:

        unit->m_numbins = numbins;
        unit->m_tempbuf = (float*)RTAlloc(unit->mWorld, buf->samples * sizeof(float));

So it's possible to have a memory leak here, where the "to/from" allocation is successful and the tempbuf failed. That could be avoided either by try/catch and checking for 0, or perhaps like this (if there is only the one RTAlloc, then there should be no memory leak).

        unit->m_to = (int*)RTAlloc(unit->mWorld, (numbins * 2 * sizeof(int)) + (buf->samples * sizeof(float)));
        unit->m_from = unit->m_to + numbins;
        unit->m_tempbuf = (float*)(unit->m_from + numbins);  // not sure of the cast here

(I don't know enough C to know if this is correct -- that approach assumes that it's easy to cast the type of a pointer. If it isn't, then it would be required to have a separate allocation per datatype.)

PartConv does 5 allocations of "fftsize" and 1 of "fullsize," all floats -- so the single-allocation approach would work.

BTW, completely side note, I noticed that PreparePartConv() uses RTAlloc -- but this is a bufgen command that runs outside the RT thread! So there is no requirement for memory allocation to be time bounded. Probably these should be changed to malloc() or whatever and avoid unnecessary use of the limited amount of RT memory.

[Spacechild1]

I would assume that a failed RTAlloc request would leave the allocator in the same state as before the request. (Is that a safe assumption?)

Yes.

Before calling Ctor(), there is no way to know whether the first, second, third etc. request will fail. So there might be some successful requests. In that case, the UGen needs to be responsible for releasing the successfully allocated blocks.

That's right.

If RTAlloc might throw an exception, then it seems a try/catch at the point of allocation would be mandatory. (There's no way for the caller to access the memory addresses to be released -- I think?)

RTAlloc must not throw an exception, it should return NULL on failure (which I think it currently does). Please read https://github.com/supercollider/rfcs/pull/18#issuecomment-989400943

An alternate approach would be to replace multiple allocations with a single, larger allocation, and apportion out the pointers as needed.

That's a common pattern. For example, when scsynth creates a new Graph, it allocates a single large block of memory which contains the Graph structure, the Wires, Controls and all the UGens. Apart from easier memory management, you also get better data locality which might result in fewer cache misses.

[jamshark70]

RTAlloc must not throw an exception, it should return NULL on failure (which I think it currently does).

AllocPool::Alloc() can throw exceptions. https://github.com/supercollider/supercollider/blob/cea67fcd49eb899366d6f7252c70157c5bc8b18f/common/SC_AllocPool.cpp#L377

[Spacechild1]

AllocPool::Alloc() can throw exceptions.

When I skimmed the code I thought that a non-growable AllocPool returns NULL if it can't allocate the desired size:

https://github.com/supercollider/supercollider/blob/cea67fcd49eb899366d6f7252c70157c5bc8b18f/common/SC_AllocPool.cpp#L383 https://github.com/supercollider/supercollider/blob/cea67fcd49eb899366d6f7252c70157c5bc8b18f/common/SC_AllocPool.cpp#L392

---

But now I made a test with a dummy plugin that calls RTAlloc in a loop and AllocPool would eventually throw an exception.

The problem is the following section: https://github.com/supercollider/supercollider/blob/cea67fcd49eb899366d6f7252c70157c5bc8b18f/common/SC_AllocPool.cpp#L360 Because of the OR, the condition will always be true. I'm not entirely sure what the purpose of this code is. I think the idea was to throw an exception if the requested size is larger than the total size of the alloc pool. Anyway, this needs to be fixed.

---

Generally, AllocPool is quite messy. Sometimes it throws exceptions, sometimes it returns 0. And some code paths are just impossible to reach: https://github.com/supercollider/supercollider/blob/cea67fcd49eb899366d6f7252c70157c5bc8b18f/common/SC_AllocPool.cpp#L391 checks the result of NewArea, although the latter throws if it couldn't allocate more memory.

In my opinion, we should remove all throw statements from AllocPool and always return NULL instead. In my view, it is a replacement for malloc and friends, so it should follow the same semantics.

It is the caller's job to decide how it should interpret and deal with the memory exhaustion. For example, the following error message really doesn't belong inside AllocPool: throw std::runtime_error(std::string("alloc failed, increase server's memory allocation (e.g. via ServerOptions)"));. Why should AllocPool - a memory allocator - need to know about Server options?

---

Finally, let me stress again that we must not throw C++ exceptions across plugin interface boundaries, so RTAlloc and RTAlloc must not throw.

Now, scsynth could internally use exceptions to deal with RT memory failure, but to quote myself:

Generally, C++ exceptions only really make sense together with RAII (https://en.wikipedia.org/wiki/Resource_acquisition_is_initialization), but scsynth is mostly written in an early "C with classes" style with manual memory management (which is kind of necessary because of the stateful allocators). Supernova is a different story, btw.

[elgiano]

Thanks for your comments, I tried to summarize main points emerged so far in the PR description above. My comments:

1. stop throwing: I'm not savvy enough to advocate for or against exceptions, but @Spacechild1 proposal to stop throwing looks good to me, especially because we would have consistent exit paths for AllocPool::Alloc, and a behavior similar to malloc. Note: we would need to do this also for Realloc, right? It can also throw.
2. single vs multiple allocations: I see this point as optimization: we might promote single allocation by providing examples in the core plugin library and/or sc3-plugins, and even think about writing a function to help doing it, but IMO this finally remains a design choice for whoever writes a plugin.
3. holding memory on rtalloc failure: I think it would be nice to release unused memory when Ctors fail, as holding it can make other UGens fail without IMO a good reason. That memory wouldn't be used anyway if Ctor failed!
4. RTAlloc in next(): The cases in server/plugins are GrainUGens and FFTUGens, including also every plugin that uses MAKE_TEMP_BUF from plugin_interface/FFTUGen.h. They are all meant to perform an initialization (runs only if alloc didn't succed before), so the main difference from allocating in Ctor is that allocations in next will intentionally retry at every processing block. We can debate if this behavior is desirable or not.

By the way, here is the command I use to find all occurrences of RTAlloc with function names:

git grep -np RTAlloc

[Spacechild1]

Note: we would need to do this also for Realloc, right? It can also throw.

Yes!

Also, we would have to look at sclang because it also uses AllocPool. However, the pool is growable and allocation only fails if the OS can't give any more memory. I haven't had a close look yet how (or if) sclang deals with out-of-memory situations.

I see this point as optimization: we might promote single allocation by providing examples in the core plugin library and/or sc3-plugins, and even think about writing a function to help doing it, but IMO this finally remains a design choice for whoever writes a plugin.

Agreed. I think some people are probably not aware of this technique, so it could be mentioned somewhere.

I think it would be nice to release unused memory when Ctors fail

Definitely! It would be a memory leak otherwise.

---

I have just compiled my test plugin with mingw64 and ran it in the "official" scsynth (which is build with MSVC). I can testify that I can't catch exceptions that are thrown from scsynth. The catch block is never called and the stack is not properly unwound (destructors of local objects are not called).

As a side note: even if the plugin could catch the exception, it would still be dangerous if it would try to call the what() method because std::exception has virtual methods and compilers use different vtable layouts. This doesn't work unless the C++ class has been designed following certain rules, like in Microsoft's COM. See also https://github.com/supercollider/supercollider/issues/4438

Summing up: a good plugin interface should be designed so that either side could also be a pure C program.

[elgiano]

I think it would be nice to release unused memory when Ctors fail

Definitely! It would be a memory leak otherwise.

Just for the sake of understanding: I don't think it would be a leak, because as long as constructors doesn't run in loops, references to allocated memory are defined only once and kept in the UGen. Then the Dtor would free that memory when the enclosing synth is destroyed. Or am I missing something?

However, I also agree that it would be better to free those blocks right away. We can discuss how to best design this at a later stage, but for now I'm thinking of something like either calling the Dtor directly or having another function for releasing allocated memory, which would be called by both the Dtor and in case RTAlloc fails.

[Spacechild1]

@jamshark70

BTW, completely side note, I noticed that PreparePartConv() uses RTAlloc -- but this is a bufgen command that runs outside the RT thread!

You're right! Ouch. Please open a GitHub issue.

[Spacechild1]

Just for the sake of understanding: I don't think it would be a leak, because as long as constructors doesn't run in loops, references to allocated memory are defined only once and kept in the UGen. Then the Dtor would free that memory when the enclosing synth is destroyed. Or am I missing something?

Assuming that the buffers are already assigned to member variables, that's true!

[elgiano]

Also, we would have to look at sclang because it also uses AllocPool. However, the pool is growable and allocation only fails if the OS can't give any more memory. I haven't had a close look yet how (or if) sclang deals with out-of-memory situations.

sclang uses Alloc in 18 files. Many of them have a system in place to check for NULL and throwing an exception. We definitely need to check this in case we remove the exception, as these places might rely on it.

Here are the ones where I couldn't see any NULL checking mechanism:

LangSource/PyrInterpreter3.cpp
LangPrimSource/PyrDeepCopier.h
LangPrimSource/PyrArchiverT.h
LangPrimSource/PyrDeepFreezer.h
---------------------------------------------------
LangSource/HashTable.h (this files seems to be never referenced by any other... dead code?)

And here I found checks only partially:

LangSource/GC.cpp (doesn't check in 1 case out of 3)
LangSource/PyrLexer.cpp (doesn't check in 5 case out of 8)

LangSource/PyrObject.cpp (doesn't check in function pyr_pool_compile_allocator::allocate, but I can't find any place where this is used)
(I need help to understand how this allocator in PyrObject works)

LangSource/PyrParseNode.h (through macros ALLOCNODE and ALLOCSLOTNODE)
(Not affected, uses AdvancingAllocPool, which null checks and throws exceptions on its own)
------------------------------------------------
LangSource/PowerOfTwoAllocPool.h [also dead code?] (doesn't check only in 1 case out of 2)

[Spacechild1]

We can discuss how to best design this at a later stage, but for now I'm thinking of something like either calling the Dtor directly or having another function for releasing allocated memory, which would be called by both the Dtor and in case RTAlloc fails.

Memory management is the responsibility of the plugin author. There are several different approaches. Actually, with C++ you could use standard containers with a custom allocator, but it has a slight memory overhead because the allocators are stateful (they need to hold a reference to World).

Don't overcomplicate things! In the destructor you would typically write:

if (mBuffer) {
    RTFree(mBuffer);
}

That's all you need, really.

[elgiano]

Memory management is the responsibility of the plugin author. There are several different approaches. Actually, with C++ you could use standard containers with a custom allocator, but it has a slight memory overhead because the allocators are stateful (they need to hold a reference to World).

I agree with let's not overcomplicate things. Talking about which, I found something like a "custom allocator" here: https://github.com/supercollider/supercollider/blob/18c4aad363c49f29e866f884f5ac5bd35969d828/include/plugin_interface/SC_InterfaceTable.h#L228

It is only used in server plugins, by FFT UGens, to call scfft_create and scfft_destroy. scfft_create does NULL checks and returns NULL, so that plugins should also NULL check after it.

[Spacechild1]

Talking about which, I found something like a "custom allocator" here: https://github.com/supercollider/supercollider/blob/18c4aad363c49f29e866f884f5ac5bd35969d828/include/plugin_interface/SC_InterfaceTable.h#L228

Yes. Unfortunately, it's a rather bad one :-) See https://github.com/supercollider/supercollider/issues/4438. Let alone it's not compatible with std::allocator.

Here's my std::allocator compliant RT allocator class: https://git.iem.at/pd/vstplugin/-/blob/master/sc/src/rt_shared_ptr.hpp I have already thought about reworking it a bit and making a PR.

[elgiano]

Great! However, for the sake of this issue alone, changing that allocator wouldn't change the relationship to exceptions and NULL checks, so we can regard it as a separate issue, right?

[Spacechild1]

However, for the sake of this issue alone, changing that allocator wouldn't change the relationship to exceptions and NULL checks, so we can regard it as a separate issue, right?

Yes. It's just another way for the plugin author to do memory management. Actually, the custom allocator approach is not without its problems. In fact, using it containers like std::vector can be risky because not all C++ stdlib implementations are able to deal with out-of-memory gracefully. To be extra safe, you would have to write your own containers. In the end, I never really used my rt::allocator class with containers, only for std::shared_ptr. Even there I'm still not sure if it's really safe and I'm tempted to replace it with my own shared_ptr class.

---

The only things we need to do is:

1. make sure that World_Alloc and World_Realloc return NULL and don't throw
2. add proper null check where necessary

---

Actually, just because World_Alloc returns NULL doesn't mean we can't still use exceptions to signal allocation failures within scsynth. However, we must not throw in the allocation function itself, but rather afterwards:

float *myBuf = (float *)World_Alloc(world, mySize);
if (!myBuf) {
    // free buffers that have been already allocated successfully
    // ...
    throw std::runtime_error(std::string("alloc failed, increase server's memory allocation (e.g. via ServerOptions)"));
}
// continue

The line with the throw statement should really be a macro or convenience function so we don't have to repeat over and over again.

---

This kind of manual memory management + exceptions only works fine if the exceptions are caught as early as possible. Currently, scsynth basically just wraps the whole audio callback in a single try block. If an exception is thrown from some function deep in the call stack, scsynth can print a nice error message, but functions lower in the call stack might not perform proper clean up if they don't use RAII. That's what people mean when they say that exceptions really go hand in hand with RAII: the basic assumption is that unwinding the stack (= calling destructors) will automatically free any temporary ressources.

One solution is to use automatic clean-up functions:

// C++17 has class template argument deduction (https://en.cppreference.com/w/cpp/language/class_template_argument_deduction)
template<T>
class CleanUp {
public:
    CleanUp(T&& fn) : mFn(std::forward<T>(fn)) {}
    ~CleanUp() { mFn(); }
    CleanUp(const CleanUp&) = delete;
    CleanUp& operator=(const CleanUp&) = delete;
private:
    T mFn;
};

Usage:

float *buf1 = nullptr;
float *buf2 = nullptr;
float *buf3 = nullptr;

// CleanUp will automatically free all buffers when it goes out of scope
CleanUp cleanup([&] () {
    if (buf1) World_Free(world, buf1);
    if (buf2) World_Free(world, buf2);
    if (buf3) World_Free(world, buf3);
});

buf1 = (float *)World_Alloc(world, mySize);
if (!buf1) {
    // should be a macro or helper function...
    throw std::runtime_error(std::string("alloc failed, increase server's memory allocation (e.g. via ServerOptions)"));
}
// same for buf2 and buf3
// ...

// finally transfer memory
mBuf1 = buf1;
mBuf2 = buf2;
mBuf3 = buf3;
buf1 = buf2 = buf3 = nullptr; // !

---

Alternatively, we could use a RAII wrapper class for RT memory, e.g.:

template<typename T>
class ScopedPtr {
public:
    ScopedPtr (World *world, T *ptr) : mWorld(world). mPtr(ptr)  {}
    // move constructor
    ScopedPtr (Scoped&& other) {
        mWorld = other.mWorld;
        mPtr = other.mPtr;
        other.mPtr = nullptr;
    }
    // move assignment
    ScopedPtr& operator = (Scoped&& other) {
        mWorld = other.mWorld;
        if (mPtr) World_Free(mWorld, mPtr);
        mPtr = other.mPtr;
        other.mPtr = nullptr;
        return *this;
    }
    // destructor
    ~ScopedPtr () {
        if (mPtr) World_Free(mWorld, mPtr);
    }
    // access pointer 
    T *get() { return mPtr; }
    // release pointer
    T *release() {
        T *ptr = mPtr;
        mPtr = nullptr;
        return ptr;
    }
    // maybe more methods, e.g. operator*, operator==, operator!=, etc.
private:
    World *mWorld;
    T *mPtr;
};

And a version of World_Alloc that throws:

void *World_AllocThrow(World *world, size_t size) {
    void *ptr = World_Alloc(world, size);
    if (!ptr) throw std::runtime_error(std::string("alloc failed, increase server's memory allocation (e.g. via ServerOptions)"));
    return ptr;
}

Now you can do the following:

ScopedPtr<float> buf1(inWorld, World_AllocThrow(inWorld, size1 * sizeof(float)));
ScopedPtr<float> buf2(inWorld, World_AllocThrow(inWorld, size2 * sizeof(float)));
ScopedPtr<float> buf3(inWorld, World_AllocThrow(inWorld, size3 * sizeof(float)));
// all allocations succeeded, now assign to member variables
mBuf1 = buf1.release();
mBuf2 = buf2.release(),
mBuf3 = buf3.release();

If one of those World_AllocThrow calls throws, all previously allocated blocks are automatically released.

This could be further simplified with a makeScopedPtr function:

template<typename T>
ScopedPtr<T> makeScopePtr(World *world, size_t n) {
    return ScopedPtr(world, World_AllocThrow(world, n * sizeof(T)));
}

Usage:

auto buf1 = makeScopePtr<float>(inWorld, size1);
auto buf2 = makeScopePtr<float>(inWorld, size2);
auto buf3 = makeScopePtr<float>(inWorld, size3);
mBuf1 = buf1.release();
mBuf2 = buf2.release(),
mBuf3 = buf3.release();

Note: this technique can also be used in plugins, but you have to make sure to wrap everything in a try block, so that the exception doesn't cross plugin boundaries. Also, ScopedPtr would need some modifications because plugins don't have access to World_Free; instead it would need to access the interface table (similar to my rt::allocator class).

Note: unlike std::unique_ptr and std::make_shared, ScopePtr and makeScopePtr don't call constructors and destructors. It's not hard to add this, but for arrays you would have to also save the number of allocated elements. That's why there is std::unique_ptr<T> (single objects) and std::unique_ptr<T[]> (arrays).

(As a side note: new T[] typically stores the size right before the allocated memory, so that delete T[] can later look it up and call the destructors in a loop. However, it only does this if T actually has a destructor to avoid the unnecessary overhead for primitive types or simply C structs. An array version of ScopePtr could use a similar optimization, although in our case it probably doesn't matter because it only lives on the stack and the compiler can figure out if the size is not used and the loop does nothing.)

---

I just wanted to give a few examples on how RAII could help dealing with these kinds of memory management issues. The current exception handling code in scsynth is definitely not optimal.

On the other hand, we don't have to use exceptions at all. We could also handle out-of-memory errors locally and propagate the error via error codes.

I don't have a real preference. The only thing I really do care about is not passing exceptions between plugins and scsynth :-)

[Spacechild1]

For the sake of completeness:

single vs multiple allocations: I see this point as optimization: we might promote single allocation by providing examples in the core plugin library and/or sc3-plugins,

A potential pitfall is alignment. On some platforms (most notably older ARM versions) misaligned pointer access can crash the program. Generally, the issue can arise if you put objects with different alignment requirements in the same memory block.

Consider the following example:

// the natural alignment of `char` is 1 byte, that of `float` is 4 bytes
size_t stringSize = strlen(someString) + 1;
size_t arraySize = n * sizeof(float);
char *mem = (char *)World_Alloc(world, stringSize + arraySize);
if (!mem) {
    // handle out-of-memory
}
char *string = mem;
mem += stringSize;
float *array = mem; // might be misaligned!

Another example:

// the natural alignment of `float` is 4 bytes, that of `float*` is 8 bytes (on 64-bit platforms)
size_t floatArraySize = n * sizeof(float);
size_t ptrArraySize = n * sizeof(float *);
char *mem = (char *)World_Alloc(world, floatArraySize + ptrArraySize );
if (!mem) {
    // handle out-of-memory
}
float *floatArray = mem;
mem += floatArraySize;
float **ptrArray = mem; // might be misaligned!

That's why we should be careful with advertizing this technique. AFAICT, even Scsynth and Supernova get this wrong: https://github.com/supercollider/supercollider/issues/5642

and even think about writing a function to help doing it, but IMO this finally remains a design choice for whoever writes a plugin.

Supernova, for example, has a simple linear allocator class (linear_allocator in utilities/utils.hpp). However, it doesn't deal with alignment, so you have to be careful.

It would be possible to change linear_allocator so that the return pointer is always properly aligned according to its type. The bigger problem is how to calculate the required total size upfront. It's not a real issue with Graphs because the size is fixed and can be read from the SynthDef, but for arbitrary memory allocations of different types this is not easy to solve...

[elgiano]

Excuse my definitely not enough experience with RAII, but I have a questions:

an approach like ScopedPtr would be mainly useful for plugins written as proper classes with proper lifecycle, because such a pointer would be destroyed at the end of the Ctor function, in UGens written as struct, right?

In this case implementing a RAII approach would mean to rewrite all the UGens? The benefit would be more automatic memory management, and the drawback a huge rewrite? Could you help evaluate if something like ScopedPtr is a realistic/desirable way to go?

[Spacechild1]

an approach like ScopedPtr would be mainly useful for plugins written as proper classes with proper lifecycle, because such a pointer would be destroyed at the end of the Ctor function, in UGens written as struct, right?

I'm not sure what "proper classes with proper lifecycle" are. Do you mean Units that have a destructor function? ScopedPtr and CleanUp are meant to be used as local variables. Their purpose is to automatically release resources when they go out of scope, avoiding the need of manual clean up if one operation fails.

In this case implementing a RAII approach would mean to rewrite all the UGens? The benefit would be more automatic memory management, and the drawback a huge rewrite? Could you help evaluate if something like ScopedPtr is a realistic/desirable way to go?

IMO, it wouldn't make much sense for UGen plugins. Most plugins that use RT memory only allocate 1 array. In this case, manual memory management is trivial. Just check the result for NULL and in the destructor check the pointer before calling RTFree.

A few plugins like BeatTrack, Convolution, PartConv, ect. allocate multiple arrays. Some of them can be combined in a single allocation (if they have the same type). Either way, it's not really that complicated:

void Foo_Ctor(Foo *unit) {
    unit->mBuf1 = RTAlloc(unit->mWorld, size1);
    unit->mBuf2 = RTAlloc(unit->mWorld, size2);
    unit->mBuf3 = RTAlloc(unit->mWorld, size3);
    if (unit->mBuf1 && unit->mBuf2 && unit->mBuf3) {
        // success
        // ...
    } else {
        // failure
        SETCALC(*ClearUnitOutputs);
    }
}

// could go to "SC_Unit.h"?
#define RTFreeCheck(unit, buf) if (buf) RTFree(unit->mWorld, buf)

void Foo_Dtor(Foo *unit) {
    RTFreeCheck(unit, unit->mBuf1);
    RTFreeCheck(unit, unit->mBuf2);
    RTFreeCheck(unit, unit->mBuf3);
}

NOTE: we also have to handle memory allocation failure in scfft_create!

[elgiano]

I'm not sure what "proper classes with proper lifecycle" are. Do you mean Units that have a destructor function? ScopedPtr and CleanUp are meant to be used as local variables. Their purpose is to automatically release resources when they go out of scope, avoiding the need of manual clean up if one operation fails.

I thought it was a way to have a Unit object own some memory so that it would get cleaned up automatically when the destructor is called. This wouldn't be "immediately" possible for all UGens written as structs (with UGenName_Ctor, Dtor and so on), but it would be for UGens written as C++ class (with ~UGenName() and so on). Anyway, it already feels too much of a stretch now to me, I agree with NULL checks being the most viable alternative for now :)

// could go to "SC_Unit.h"? #define RTFreeCheck(unit, buf) if (buf) RTFree(unit->mWorld, buf)

Coincidence, just for "fun" I've made such a macro yesterday and applied all over server/plugins and sc3-plugins. I was also looking into another macro to automate RTAlloc, NULL-check, SETCALC and printing an error message

#define RTAllocOrClearUnit(ptr, size)                                                                                  \
    ptr = (decltype(ptr)) RTAlloc(unit->mWorld, size * sizeof(*ptr));                                                  \
    if (ptr == nullptr) {                                                                                              \
          printf("%s: real time memory allocation failed\n", __func__);                                                \
          SETCALC(*ClearUnitOutputs);                                                                                  \
          unit->mDone = true;                                                                                          \
          return;                                                                                                      \
    } 

#define RTFreeSafe(world, ptr) if (ptr != nullptr) RTFree(world, ptr) 

How does it look to you? And i put them in InterfaceTable.h instead of SC_Unit.h, because I put them next to RTAlloc and RTFree. But it would also make sense to have them in SC_Unit.h, since they're only relevant to Units (at least the first one is for sure)

[Spacechild1]

I thought it was a way to have a Unit object own some memory so that it would get cleaned up automatically when the destructor is called.

Every Unit that allocates memory with RTFree in the constructor must already have a destructor where the memory is freed is RTFreed. The only difference would be to check for NULL in case the allocation has failed. (Some plugins already do this.)

This wouldn't be "immediately" possible for all UGens written as structs (with UGenName_Ctor, Dtor and so on), but it would be for UGens written as C++ class (with ~UGenName() and so on).

It's perfectly possible with C structs. You declare a destructor function and register the Unit with DefineDtorUnit instead of DefineSimpleUnit. The C++ wrapper is just syntactic sugar. Have a look at DelayUGens.cpp, for example.

---

#define RTAllocOrClearUnit(ptr, size)                                                                             \
  ptr = (decltype(ptr)) RTAlloc(unit->mWorld, size * sizeof(*ptr));                                                  \
   if (ptr == nullptr) {                                                                                 \
         printf("%s: exception in real time: %s\n", __func__, e.what());                                              \
         SETCALC(*ClearUnitOutputs);                                                                                  \
         unit->mDone = true;                                                                                          \
         return;                                                                                                      \
   }

Where does e.what() come from? RTAlloc shouldn't throw. I guess it should rather be something like: printf("%s: alloc failed, increase server's RT memory (e.g. via ServerOptions)", __func__);

Apart from that, it looks nice!

Note that if you have to do multiple allocations in a row, you would need to set all pointer members to NULL before the first allocation. Otherwise you would end up with wild pointers if one of the allocation fails because you would return from the constructor function before assigning the remaining members. As a consequence the destructor would crash by passing a bad pointer to RTFree. Example:

struct Foo : Unit {
    float *mBuf1;
    float *mBuf2;
    float *mBuf3;
};

void Foo_Ctor(Foo *unit) {
    unit->mBuf1 = unit->mBuf2 = unit->mBuf3 = nullptr; // !
    // ...
    RTAllocOrClearUnit(unit->mBuf1, size1);
    RTAllocOrClearUnit(unit->mBuf2, size2);
    RTAllocOrClearUnit(unit->mBuf3, size3);
    // ...
}

void Foo_Dtor(Foo *unit) {
    RTFreeSafe(unit->mWorld, unit->mBuf1);
    RTFreeSafe(unit->mWorld, unit->mBuf2);
    RTFreeSafe(unit->mWorld, unit->mBuf3);
}

PluginLoad(Foo) {
    ft = inTable;
    DefineDtorUnit(Foo);
}

[elgiano]

Where does e.what() come from?

Sorry, was a leftover from the previous version, which caught an exception. I edited it out now.

you would need to set all pointer members to NULL before the first allocation. Otherwise you would end up with wild pointers if one of the allocation fails because you would return from the constructor function before assigning the remaining members. As a consequence the destructor would crash by passing a bad pointer to RTFree

That's a very good point! Can we make sure every pointer is initialized as NULL by doing it in the struct definition? Example:

struct Pitch : public Unit {
    float m_values[kMAXMEDIANSIZE];
    int m_ages[kMAXMEDIANSIZE];
    float* m_buffer = NULL; // here we go

    float m_freq, m_minfreq, m_maxfreq, m_hasfreq, m_srate, m_ampthresh, m_peakthresh;
    int m_minperiod, m_maxperiod, m_execPeriod, m_index, m_readp, m_size;
    int m_downsamp, m_maxlog2bins, m_medianSize;
    int m_state;
    bool m_getClarity;
};

NOTE: we also have to handle memory allocation failure in scfft_create!

True, and I had already added a note about it in the main PR text above :)

[Spacechild1]

make sure every pointer is initialized as NULL by doing it in the struct definition?

Only with the C++ wrapper (SCUnit) where have a real constructor that would (automagically) make those assignments before executing our constructor code.

With Unit there is no actual constructor, only a user defined function, so any in-class member initializers are ignored.

[elgiano]

Also, about releasing eventual previously allocated memory when one RTAlloc fails, I'm suspecting it wouldn't be a good idea to call the Unit Dtor directly. Especially in UGens written as cpp classes, I wouldn't feel comfortable calling their ~Dtor() manually. Is it just superstition? Or shall we do it?

[Spacechild1]

Also, about releasing eventual previously allocated memory when one RTAlloc fails, I'm suspecting it wouldn't be a good idea to call the Unit Dtor directly.

That would definitely be a bad idea :-)

There are generally two different strategies:

1. first store the results in local variables and only assign to the struct members if all allocations have succeeded. This implies that after an allocation failure we would have to manually free all memory that has already been allocated successfully.
2. directly store the results in the member variables and let the destructor free them (if necessary)

IMO, 2 is much more convenient. 1 has the "advantage" that memory is released as soon as possible, but I don't think it warrants the extra complexity.

While it would be possible to manually call the destructor to "speed up" 2, I think it would be rather weird. Also we would need extra code to ensure that the destructor is not called again when the Unit is freed. I would say just forget about it :-)

[elgiano]

While it would be possible to manually call the destructor to "speed up" 2, I think it would be rather weird. Also we would need extra code to ensure that the destructor is not called again when the Unit is freed. I would say just forget about it :-)

I agree, let's let the Dtor do its job as usually, when the Synth is destroyed. If the UGen has a mDone flag that can be set, users can have Synths destroy themselves when mDone is set, so also in case of failed memory allocation. I think that would be enough!

With Unit there is no actual constructor, only a user defined function, so any in-class member initializers are ignored.

So we'll have to ensure all pointers are initialized to NULL before any RTAlloc is performed in a Ctor. It sounds like a lot of work, especially for sc3-plugins, while it sounds quite agile to me to do it on server/plugins. Anyway, if we need to do it, we'll do it :)

[Spacechild1]

So we'll have to ensure all pointers are initialized to NULL before any RTAlloc is performed in a Ctor.

Yes, but only if there is more than one allocation. Most plugins that need RT memory only make a single allocation. Some make two allocations and only a few make more (most notable the convolution UGens).

Equally important is adding proper NULL checks in the destructors which many plugins are missing...

[Spacechild1]

BTW, here is an alternative pattern:

#define ClearUnitOnFailure \
    printf("%s: alloc failed, increase server's RT memory (e.g. via ServerOptions)", __func__);  \
    SETCALC(*ClearUnitOutputs);                                                                                         \
    unit->mDone = true;                                                                                                     \
    return;

void Foo_Ctor(Foo *unit) {
    unit->mBuf1 = RTAlloc(unit->mWorld, size1);
    unit->mBuf2 = RTAlloc(unit->mWorld, size2);
    unit->mBuf3 = RTAlloc(unit->mWorld, size3);
    if (!(unit->mBuf1 && unit->mBuf2 && unit->mBuf3)) {
        ClearUnitOnFailure
    }
    // ...
}

Compare this with the first pattern:

void Foo_Ctor(Foo *unit) {
    unit->mBuf1 = unit->mBuf2 = unit-mBuf3 = NULL;
    RTAllocOrClearUnit(unit->mBuf1, size1);
    RTAllocOrClearUnit(unit->mBuf2, size2);
    RTAllocOrClearUnit(unit->mBuf3, size3);
    // ...
}

Both do the same thing and it's only a matter of taste. Personally, I don't have a real preference.

[elgiano]

Equally important is adding proper NULL checks in the destructors which many plugins are missing...

And this can be done by the RTFreeSafe or RTFreeCheck macro

Both do the same thing and it's only a matter of taste. Personally, I don't have a real preference.

I like more the first one, because it fails immediately, so you can't do memset(NULL, ...) or something like that.

So it looks like we have somewhat of a plan for now:

1. double check that server and sclang don't need to differentiate between NULL-returning and exception-throwing exit paths from RTAlloc
2. remove the throw from RTAlloc
3. put NULL checks in server and sclang, where the exception can be thrown again
4. implement macros for safer RTAlloc/RTFree in plugins (and sc3-plugins)
5. for UGens which do multiple allocations, ensure all member pointers are initialized to NULL before the first allocation
6. patch scfft_alloc as well
7. figure out what to do with UGens calling RTAlloc in next(). 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)
8. Update documentation for devs to know how to use RTAlloc with new macros in the future

[elgiano]

I'll update the PR text above and let this RFC rest until the end of the month. If there are no further developments, I think we can go to Final Comment Period in early January, and then start working on a PR to fix this!