fix(DlgPrefController): add missing layout

[acolombier]

Fix asserts introduced in #14006 and deadlock introduced in #14159

[acolombier]

I suspect this fix is incomplete, but I'm not sure what the Qt::BlockingQueuedConnection was intending to fix. @daschuer do you remember?

[daschuer]

Yes, I know. Probably the comment was not clear enough. We have two copies of the mapping, one is the shared copy in the preferences, the user can edit. The other is the immutable copy that is used in the controllers thread.
We must prevent that the mapping is edited in the main thread while copying it. Else we have a corrupted copy that may lead to a crash.

The blocking connection stops the main thread that the mapping cannot he edited.

I think a solution her is to introduce two calls. One copying the mapping and a second one that interacts with the GUI thread after we have the immutable copy.

[acolombier]

Do you think my fix is sufficient then? IIUC, this change will load and bind the GUI component once the controller has acknowledge the newly copied mapping. Or do it it further changes?

[daschuer]

Without the blocking connection this:

mixxx/src/controllers/midi/midicontroller.cpp

Line 60 in 476123b

m_pMapping = downcastAndClone<LegacyMidiControllerMapping>(pMapping.get());

Is called from the controller thread while the GUI thread has still write access to the mapping.

A workaround would be to restore the blocking call and do the controller GUI initialization in a second slot without a lock.

The copying is done in the controller thread, because the controller thread is this way also like locked implicit.

An alternative would be to clone the mapping twice.
First create a copy in the GUI thread. Than we have a temporary immutable mapping. Than copy it again in the controller thread by swapping pointers.

I like to avoid locking in the controller thread, because the mapping loockups happen on a high rate.

[acolombier]

A workaround would be to restore the blocking call and do the controller GUI initialization in a second slot without a lock.

This is not possible. Here is sequence diagram of the event loop

sequenceDiagram
    GUI->>Controller: applyMapping
    activate Controller
    critical setup resources in main thread
    Controller-->>GUI: setup resources in main thread
    end

Loading

This means that there is no way we can keep applyMapping a blocking call and keep this working. Please bare in mind that the lock in controller is out of our remit (see doc)

I like to avoid locking in the controller thread, because the mapping loockups happen on a high rate.
I'm not sure to understand what this has to do here? The deadlock doesn't happen at loockup but at applyMapping (a.k.a controller initailisation)

Basically, we use applyMapping in 3 occurrences in the preference screens:

• MIDI mapping (which seems to justify you adding that blocking connection)
• Mapping selection (which will deadlock if loading a screen mapping
• Default mapping selection (which will deadlock if loading a screen mapping

[daschuer]

The lockinh needs to be installed whenever the mapping is accessed, because of of these calls can be happen and it causes a crash.

A workaround would be to restore the blocking call and do the controller GUI initialization in a second slot without a lock.

This is not possible. Here is sequence diagram of the event loop

For my understanding the workflow can remain. We just need to take care that the mapping is not altered during setting up the resources in main. That's why I think that one blocking call for the mutable stuff and one other call for the rest will work.

Whenever the user changes a preferences option that affecte the mapping, we need to repeat the trick. Editing the mapping in the GUI thread an apply in the blocking call.

[acolombier]

Could we add a mutex to slotApplyMapping then? Since the deadlock comes from blocking the whole thread's event loop, restricting the critical section should fix this issue

[JoergAtGithub]

Could you please retarget the fix for the deadlock introduced by #14159 to 2.5.

[acolombier]

Done - #14006 to be addressed separately

[acolombier]

Actually, I reverted the PR for 2.6. The deadlock only impact that branch as this is related to QML screen controller (see comment here), so we could release 2.5.1 without this change. WDYT?

[JoergAtGithub]

Ok, makes sense!

[JoergAtGithub]

Please resolve the merge conflicts!

[acolombier]

Can you show a backtrace form the dead lock situation?

You can see a backtrace here

[acolombier]

Okay, so let's sum up where we have:

• We must prevent concurrent access to the controller mapping, which occur between the main thread and the controller thread
• We cannot block the main thread as it will need its eventloop to be running to allow offscreen rendering resources to be created when ControllerManager::openController is called.

This means we cannot using Qt::BlockingQueuedConnection, which will block the thread (and thus the eventloop) till the controller slot has completed.

Here are the solutions I can thing about:

1. Ensure concurrent access at the LegacyControllerMapping level by introducing a lock, similar to how Track works
2. Block the main thread with explicit means (not Qt::BlockingQueuedConnection) and introduce a new signal to be emitted when ControllerManager::openController starts, which would lift the lock on main
3. Duplicate the LegacyControllerMapping and remove the shared_ptr, leading to concurrent access across threads

1) analysis:

• Pro: we ensure no change on the shared access design that seems to be used in various place
• Neutral: no performance impact because mapping are not accessed/edited at runtime and is only a one-off when starting a controller.
• Con: we add a lot of boilerplate, similar to what there is in Track

2) analysis:

• Pro: we ensure no change on the shared access design that seems to be used in various place
• Neutral: no performance impact because mapping are not accessed/edited at runtime and is only a one-off when starting a controller.
• Con: we add another signal a strong dependency on the main thread locking state.

3) analysis:

• Pro: Kill the concurrent access risk at the root, without locking overhead
• Con: technically breaking change to the API, with a risk of losing the currently assumed shared instances. Hard to confirm it won't add any regression

From this, it feels like the "least worse" option in order would be:

• 2)
• 1)
• ... and 3) (major change on a release candidate!)

wdyt?

[daschuer]

We already have 3. for editing the mappings itself. So a natural solution would be to extend this for the controller preferences as well.

Con: technically breaking change to the API, with a risk of losing the currently assumed shared instances. Hard to confirm it won't add any regression

Can you explain that a bit more? Which API will be changed?

I think we need to separate two things. Applying the preferences to the JS Code and on the fly changes of values. Do we have the later? For my understanding we always apply all settings at once.

[acolombier]

We already have 3. for editing the mappings itself. So a natural solution would be to extend this for the controller preferences as well.

Can you clarify this? I understand that there is some space where we clone the mapping, but these are localised usecase. Are we sure this won't have any impact otherwise?

Can you explain that a bit more? Which API will be changed?

Sure, my fear is that current we rely on a shared_ptr<Mapping> instance, which implies that the data in shared between the Controller and main. I don't have a full picture on why and how this is currently - for example, no idea if this is potentially used during the point-n-click of MIDI learning, where the controller and main rely on a single mapping instance?
This shared ptr is currently used in many cases (all controller types impl) and beside my moderate experience on this part of the code, I cannot say for sure if removing this shared_ptr would break sync of the data across thread.

If you are 100% sure it won't, I'm happy to go down this road.

I think we need to separate two things. Applying the preferences to the JS Code and on the fly changes of values. Do we have the later? For my understanding we always apply all settings at once

Oh yeah, for now it's the latter, tho there was discussions to bring the former as an option in the future (and a split would likely cripple this feature opportunity)

[daschuer]

Can you clarify this?

We had an issue with writing a used copy of the mapping which was fixed here:
#14159

I have tried to explain how it works in the commit description. Is this understandable of do you have questions?
I think we can treat the mapping options in the same way like the mapping itself and have also a immutable copy when running.

Sure, my fear is that current we rely on a shared_ptr instance, which implies that the data in shared between the Controller and main.

This is used for ownership tracking only. I think this is the significant part:

mixxx/src/controllers/bulk/bulkcontroller.cpp

Lines 100 to 103 in 6adb012

	void BulkController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
	m_pMutableMapping = pMapping;
	m_pMapping = downcastAndClone<LegacyHidControllerMapping>(pMapping.get());
	}

The controller receives from the settings a shared pointer which tracks that it is not deleted when one thread outlives the other. In addition we do a deep copy into std::unique_ptr<LegacyHidControllerMapping> m_pMapping which is than used excursively from the controller thread.

If you are 100% sure it won't, I'm happy to go down this road.

I don't have a crystal ball to be that. :-/

Oh yeah, for now it's the latter, tho there was discussions to bring the former as an option in the future (and a split would likely cripple this feature opportunity)

Like in the mapping itself the values are still mutable only the control structures are not. So runtime changes can be still applied via atomic variables inside the otherwise immutable version of the controller preferences. Like control objects. (or reuse them). The crasher here does is not cased by access to preferences values it is cased because the whole preferences structure has changed behind the back.

[acolombier]

I tried to implement the cloning for the last 2h, but couldn't get anything to work that wouldn't be a major refactor. Would you want to push what you had in mind maybe?

[daschuer]

Can you point me to you experimental branch which not works? Why does cloning not work?

[acolombier]

Unfortunately, I didn't commit everything and discarded all the changes (twice actually, after hitting round blocks)
The issue was related to the sheer size required by updating the current widely used shared_ptr to unique_ptr/const ptr*. I didn't get to a state I could test something but I was getting 300+ line changes.

I guess I could keep the shared_ptr for now and just do the clone in each Controller::setMapping (which has downcastAndClone), tho there would still be some potentially concurrent access in the ControllerManager slot.

[ywwg]

I am following this PR with interest would like to understand these issues better and try to help work out a solution. Would you two be open to doing a call to look at the code in depth? Concurrency is one of the things Mixxx struggles with and we want to ensure we don't create technical debt or bad bugs.

[acolombier]

Good shout, I'm up for it. Will create a thread on Zulip!

[acolombier]

Superseded by #15524

[acolombier]

Actually, there is 2ed412f that still need merging!

[daschuer]

LGTM