MidiKeyboardState is not ThreadSafe

bool MidiKeyboardState::isNoteOn (const int midiChannel, const int n) const noexcept
{
    jassert (midiChannel >= 0 && midiChannel <= 16);

    return isPositiveAndBelow (n, 128)
            && (noteStates[n] & (1 << (midiChannel - 1))) != 0;
}

bool MidiKeyboardState::isNoteOnForChannels (const int midiChannelMask, const int n) const noexcept
{
    return isPositiveAndBelow (n, 128)
            && (noteStates[n] & midiChannelMask) != 0;
}

These two functions trigger warnings in xcode’s Thread Sanitizer
noteStates is being accessed on the GUI thread while it is being locked here during noteOn messages:

void MidiKeyboardState::noteOn (const int midiChannel, const int midiNoteNumber, const float velocity)
{
    jassert (midiChannel >= 0 && midiChannel <= 16);
    jassert (isPositiveAndBelow (midiNoteNumber, 128));

    const ScopedLock sl (lock); //Locked here

    if (isPositiveAndBelow (midiNoteNumber, 128))
    {
        const int timeNow = (int) Time::getMillisecondCounter();
        eventsToAdd.addEvent (MidiMessage::noteOn (midiChannel, midiNoteNumber, velocity), timeNow);
        eventsToAdd.clear (0, timeNow - 500);

        noteOnInternal (midiChannel, midiNoteNumber, velocity);
    }
}

void MidiKeyboardState::noteOnInternal  (const int midiChannel, const int midiNoteNumber, const float velocity)
{
    if (isPositiveAndBelow (midiNoteNumber, 128))
    { //accessed here:
        noteStates[midiNoteNumber] = static_cast<uint16> (noteStates[midiNoteNumber] | (1 << (midiChannel - 1)));

        for (int i = listeners.size(); --i >= 0;)
            listeners.getUnchecked(i)->handleNoteOn (this, midiChannel, midiNoteNumber, velocity);
    }
}

since processNextMidiEvent is public, the lock in processNextMidiBuffer should be moved to that function. Otherwise processNextMidiEvent is not a thread-safe function when called by itself…

Is there a particular reason these locks were not added to these functions?

edit: ok, so locking those two functions would cause priority inversions when the MidiKeyboardComponent calls them.
however, the TSan issues go away if you change noteStates to this type and update the functions in MidiKeyboardState to use the Atomic<> getters:

Atomic<uint16> noteStates[128];

Thanks for reporting! This is fixed on the develop and juce6 branches now.

1 Like

Hey, I just wanted to report that the shouldCheckState member in this class should probably be Atomic.

At one point, I had made it atomic and it cured a ThreadSanitizer issue I was having.

the MidiKeyboardComponent::handleNoteOn and handleNoteOff functions use it as does timerCallback()

Thanks!