8FACE causes Entrian Acoustic Drums to crash

[RichieHindle]

As I understand it, 8FACE calls ModuleWidget->fromJson(...); from its own worker thread. This seems a bit dangerous, and it's causing one of my modules to crash.

(Simplified details, in case they're helpful: imagine a module has a member variable thing, and two lines of code like this in fromJson():

delete thing;
thing = new Thing();

That code is normally only run in the main UI thread of rack. Elsewhere, in draw() for example, is a line of code like this:

thing->draw();

That's also only normally called from the main UI thread. All is well, but then 8FACE calls fromJson() from its worker thread, thing->draw() gets called from the UI thread at the same time, and the module crashes because the drawing code is accessing a deleted object.)

Based on a quick skim of your ChangeLog and issues, it seems like you added the worker thread to avoid issues with some modules, but it has since caused issues with others (VCV Chords, at least). Would it be possible to make the worker thread an optional feature, disabled by default? (I'm surprised it doesn't cause problems with more modules!)

[RichieHindle]

I realise I may have misunderstood the change that introduced the worker thread... previously, was ModuleWidget->fromJson(...) called from the audio thread? That's also likely to cause the same problems, so my request to make that change optional wouldn't help. I believe the only safe way to do this is to call ModuleWidget->fromJson(...) from the UI thread.

[stoermelder]

Hi Richie, thanks for your investigations. You are absolutely right about potential problems of using a worker thread for loading presets and I was aware about that when I implemented it. I know my module is doing stuff that isn't 100% stable. The thing is, even calling it from the UI thread may cause problems because the call is unsychronized to the DSP thread and some variables may change in the middle of a process call. Still, you are right about issues on uninitialized variables in draw() if delete and new are used in fromJson().

The problem with the UI thread is that is dependent on the screen refresh rate set by the user. On my laptop I'm running Rack at 20Hz which would cause some delay on preset load. In prospect of Rack v2 with headless mode and most probably no UI thread it wouldn't work at all.

I created an issue on Rack's repository about a similar issue some days ago but my proposed design may no solve this problem completely. https://github.com/VCVRack/Rack/issues/1656

I'm afraid currently I can only think of adding a blacklist and putting your module onto it to avoid using 8FACE with it and causing further crashes. What do you think?

[RichieHindle]

even calling it from the UI thread may cause problems

I don't think that's true. From the module's point of view, there's no difference between a call to fromJson() from the user pasting a preset, and 8FACE calling fromJson() from the UI thread. The lack of synchronisation between the UI thread and the audio threads is already something that modules have to deal with.

Your suggestion in https://github.com/VCVRack/Rack/issues/1656 would fix this problem for conflicts with the audio threads, but not for conflicts with the UI thread. I've seen Acoustic Drums crash in an audio thread because the rug has been pulled out from under it, and https://github.com/VCVRack/Rack/issues/1656 would fix that, but it wouldn't fix the draw() example.

You're right about headless mode. If there was only one audio thread you'd be OK - you'd call fromJson() from the audio thread and there'd be no other threads to conflict with. I can't think of a solution that works with multiple audio threads other than https://github.com/VCVRack/Rack/issues/1656.

A blacklist of modules that 8FACE refuses to work with would be harsh - how about a list of modules that require fromJson() to be called from the UI thread? That would come at the cost of one UI frame of latency, but that wouldn't matter in the majority of cases, and it's much better than refusing to work at all. Maybe in the case of headless v2 with multiple audio threads it will have to refuse, but that's for the future, by which time https://github.com/VCVRack/Rack/issues/1656 might have happened.

I've added this feature to my checkout of 8FACE: presetLoad() checks the module widget against a list, and if there's a match it uses the UI thread to load the preset instead of the worker thread. It all works very well. I'd be happy to send you a pull request (or just a patch file) if you think it might be a good way to go.

[stoermelder]

I finally implemented this for 8FACE and the upcoming 8FACE mk2.

[RichieHindle]

That's great - thanks!

But I don't think you've added Entrian Acoustic Drums to the list of affected modules - you have Entrian-Sequencers and Entrian-Free, but those are different plugins. You also need Entrian-AcousticDrums:

 std::make_tuple("Entrian-AcousticDrums", "AcousticDrums"),
 std::make_tuple("Entrian-AcousticDrums", "Drummer"),

(You're right to have Entrian-Sequencers and Entrian-Free in that list as well - please don't remove them.)

You're more than welcome to a free license for any of my modules - just let me know if you'd like one. :-)