EXC_BAD_ACCESS when pushing to AbstractFifo

I wrote this template class for creating lock-free FIFOs of arbitrary type, based on AbstractFifo:

template <typename T, size_t capacity>
struct GenericFifo
{
    bool push (T element)
    {
        auto write = fifo.write (1);

        if (write.blockSize1 > 0)
        {
            buffer[write.startIndex1] = element;
            return true;
        }
        else if (write.blockSize2 > 0)
        {
            buffer[write.startIndex2] = element;
            return true;
        }
        else
            return false;
    }

    bool pull (T& element)
    {
        auto read = fifo.read (1);

        if (read.blockSize1 > 0)
        {
            element = buffer[read.startIndex1];
            return true;
        }
        else if (read.blockSize2 > 0)
        {
            element = buffer[read.startIndex2];
            return true;
        }
        else
            return false;
    }

    juce::AbstractFifo fifo { capacity };
    T buffer[capacity];
};

I’m using it in a plugin project to push MidiMessages to the MidiBuffer in AudioProcessor::processBlock when a Button is clicked in the GUI.

So in my AudioProcessor, I have:

GenericFifo<juce::MidiMessage, 64> midiInputQueue;

And in AudioProcessor::processBlock:

juce::MidiMessage m;
while (midiInputQueue.pull (m))
    midiMessages.addEvent (m, 0);

And in the Button's onClick lambda in my AudioProcessorEditor, I have:

while (! processor->midiInputQueue.push (midiMessage));

Now, when I click the Button, sometimes it works. Other times, I get an EXC_BAD_ACCESS in AbstractFifo::finishedWrite. The fact that it only fails sometimes makes me think there is some kind of data race going on?

When I debug at the point of failure, it says oddly that midiInputQueue.fifo.bufferSize is 0. But placing a breakpoint in the AudioProcessor constructor shows that it was indeed initialized to 64.

Does anyone have an idea what’s going on here? I’m not sure how else to debug this…

Not sure the point of using the abstract fifo here you may be better off with just a vector push & pop or something?

in your push method you’re returning before you ever call finishedWrite so your circular buffer will never move forward. likewise in your pull method.

Not sure on the crash though…

Right, my bad! I updated the initial post to use the RAII classes in AbstractFifo, which should automatically call finishedRead/finishedWrite when they go out of scope. Still getting the same error though…

And I decided to use AbstractFifo because I needed to update the queue indices atomically + not lock the audio thread (using an std::vector would cause allocation/deallocation with each push/pop, no?). Am I missing something?

Ahh true yeah that’s a good point!

& nice I hadn’t seen those new methods yet!

That is really funky. I’m not super experienced on templated classes but sounds like things are not constructing how they seem 0.o maybe those initializers are not guaranteed to construct the class members like that properly?

Shouldn’t you use MidiMessageCollector for that?

https://docs.juce.com/master/classMidiMessageCollector.html

Also, you should use std::array<T, capacity> buffer;

Shouldn’t you use MidiMessageCollector for that?

Ah, yeah… that sounds like the reasonable thing to do :slight_smile: I guess I was hoping for something lock-free, and a little more lightweight, since I don’t need the MidiKeyboardState or MidiInput functionality. But I’m just being picky :sweat_smile:

Also, you should use std::array<T, capacity> buffer;

I’m curious, why?

it’s a cost-free container that is safer to use than a raw array.

Shouldn’t you use MidiMessageCollector for that?

So I tried this — in AudioProcessor I added a public member:

juce::MidiMessageCollector midiMessageCollector

Then in AudioProcessor::prepareToPlay I call:

midiMessageCollector.reset(sampleRate);

And in AudioProcessor::processBlock I call:

midiMessageCollector.removeNextBlockOfMessages(midiMessages, buffer.getNumSamples());

And lastly in my Button’s onClick lambda I call:

processor->midiMessageCollector.addMessageToQueue (midiMessage);

BUT… I get an assertion failure whenever I click the Button, at juce_MidiMessageCollector.cpp:54

jassert (hasCalledReset); // you need to call reset() to set the correct sample rate before using this object

… even though reset was already called in prepareToPlay… this is very strange…

Got it fixed! Credit goes to @adamski for identifying the problem:

I was declaring my Button::onClick lambdas using capture by reference:

button.onClick = [&] { ... };

When I called on processor (which shadows this->processor) in the lambda, it captured a reference to this, which only stays alive for the scope of the function (this has the same value but a different memory address in different member functions within an object).

I needed to capture this by value instead:

button.onClick = [this] { ... };
2 Likes

You should capture this with a Component::SafePointer<>, in case your lambda is called in the odd instance where your editor window has closed. it’s a common issue with asynchronous programming. you must be certain your captured variables are still valid when the lambda is invoked before using them.

4 Likes