Remove note from m_playingNotes before deleting m_pluginData

[midi-pascal]

fixes #2401: Random crash in sf2_player

[tresf]

Much cleaner, thanks!

FYI - If you modify the PR description to contain "fixes #2401" or "closes #2401", the associated bug report will be closed as soon as this PR is merged.

Also, as I'm sure you can observe, the PR title is getting cut off because if it is too long, this is something to consider for future commits or PRs to avoid the ... ellipses being added to the end. 👍

[midi-pascal]

Done!

[tresf]

👍

FYI - "fixes" will need to go in the description, not the title for auto-close to work.

[midi-pascal]

Done (Take 2)

[tresf]

@michaelgregorius @Wallacoloo any thoughts on merging? 👓

[Wallacoloo]

There's the possibility that m_playingNotes.indexOf( _n ) >= 0 evaluates to true, then another thread removes _n, and then we obtain a lock and attempt to call m_playingNotes.remove( m_playingNotes.indexOf( _n ) ), which would be an error.

The simple solution is to move the locking and unlocking to just outside of the if statement.

[Wallacoloo]

Although, maybe that's not a race condition that you're trying to protect against (as the other note removals don't have these checks). Though you still should lock m_playingNotes before calling indexOf in case another thread causes the vector to be resized before indexOf returns. So in either case, the locking should be moved to outside the conditional.

[Wallacoloo]

@tresf I left my feedback. The code inside these lock statements doesn't have the opportunity to hang or call another function that will attempt to obtain other locks, so it looks to be safe and deadlock-free.

[michaelgregorius]

@tresf If I understand correctly this patch is rather a band-aid than fixing the "real" cause of the problem but I think it is still better than having nothing. I investigated the problem together with @midi-pascal in #2401 but unfortunately it is hard to find the root cause (as is most of the times with threading related problems). So with regards to the goal of having a version 1.2 I'd say we should merge this as it provides a solution to the problem. There is always room for improvement later.

@midi-pascal Thanks for your endurance in investigating this problem! 😃

[midi-pascal]

@Wallacoloo I did what you suggested.
@tresf Not sure if I squashed the 2 commits correctly. Can you verify, please?

[midi-pascal]

@michaelgregorius Thanks! This one was tough to find, even if I did not find its root!

[tresf]

@midi-pascal, try rebase again with HEAD~2.

If you click on the commits tab above, you'll see how many commits you have.

[midi-pascal]

@tresf I already tried this but I get:

This is a combination of 2 commits.
The first commit's message is:

Move m_playingNotesMutex.lock() and m_playingNotesMutex.unlock() outside of if( m_playingNotes.indexOf( _n ) >= 0 )

Conflicts:
plugins/sf2_player/sf2_player.cpp
......
So git sees a conflict so I cannot commit later.

[midi-pascal]

@tresf I think I got it.

[tresf]

Great. Consensus says 👍, merging.

[midi-pascal]

😄