Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QML thread affinity check is missing #83

Closed
mchistovib opened this issue Sep 28, 2022 · 10 comments
Closed

QML thread affinity check is missing #83

mchistovib opened this issue Sep 28, 2022 · 10 comments

Comments

@mchistovib
Copy link

mchistovib commented Sep 28, 2022

Describe the bug
Thread affinity check is not done for qml in setip that I will provide in an example below:
https://github.com/mchistovib/brokenQMLThreadAffinity
In example, just attach some jjambi jars and QT lib and click on "add rows other thread" button

To Reproduce
Steps to reproduce the behavior:

  1. Create qml table that uses model from java code passed using setConext
  2. Now trigger any usage of that model that interacts with ui(for example, adds/removes rows) to run from another thread
  3. Observe instant jvm crash

Expected behavior
Since jambi thread affinity checks are enabled, we expect a nice java exception being thrown, like in other places where we got these thread affinity issues.

System (please complete the following information):

  • OS: Windows 10
  • Java version Zulu 11
  • QtJambi version 6.2.7
  • Qt version 6.2.5
@mchistovib
Copy link
Author

@omix also qrc:/ links stopped working with latest jambi build that I made, could you please check if they work for you? In the IDE mode, where all files are added to classpath from build folder

@omix
Copy link
Contributor

omix commented Sep 29, 2022

I can clearly say, that the issue you reported is not caused by QtJambi but by Qt. In general, threaded changing of models is possible. There is no affinity check because it is allowed to edit the table in another thread. The thread sends change notification via signals and these are sent in the owner thread of the receivers.

I verified this by using a widget-based table view:

    	QMainWindow mainWindow = new QMainWindow();
    	QTableView widget = new QTableView();
    	Controller controller = new Controller();
    	widget.setModel(controller.getModel());
    	mainWindow.setCentralWidget(widget);
    	mainWindow.menuBar().addMenu("file").addAction("add rows other thread").triggered.connect(controller::addOtherThread);
    	mainWindow.show();

Works well.
I also tried to figure out where the problem lies. It seems to be the connection between TableView and HorizontalHeaderView.
By removing the HorizontalHeaderView from qml (or the connection syncView: tableView) it works well.

@mchistovib
Copy link
Author

We also have a similar looking crash for webengineview in qml, happens not every time when you change webapp contents fast. perhaps it's releated

@omix
Copy link
Contributor

omix commented Sep 30, 2022

@omix also qrc:/ links stopped working with latest jambi build that I made, could you please check if they work for you? In the IDE mode, where all files are added to classpath from build folder

It seems, QtJambi does not load from directory. I'll fix it next version.
There is a workaround: use QResource.addSearchPath() with a URL to your directory. The URL must end with a slash:

QResource.addSearchPath("file:///C:/my/path/");

omix added a commit that referenced this issue Oct 15, 2022
Bugfix Issue #89
Bugfix Issue #86
Bugfix Issue #85
Bugfix Issue #84
Bugfix Issue #83
Bugfix Issue #79
Bugfix Issue #78
Bugfix Issue #77
Bugfix Issue #75
@omix omix closed this as completed Oct 15, 2022
@omix
Copy link
Contributor

omix commented Oct 24, 2022

Reopening because of response in https://bugreports.qt.io/browse/QTBUG-107598

@omix omix reopened this Oct 24, 2022
@mchistovib
Copy link
Author

mchistovib commented Oct 25, 2022

@omix I think QT guy is right, look at qt beginInsertRows implemntation:
image
By default, no one is listening for emitted signal, so it doesn't matter that it is emitted not from the main thread. All other code in the method is thread safe:
image

However, once you use HeaderView, it makes it's proxyModel to listen to all signals of the main model:
image
So at the moment any abstractModel method that emits signal that proxyModel is listening to, thread affinity crash is happening.

@omix
Copy link
Contributor

omix commented Oct 25, 2022

Methods QAbstractItemModel::beginX and QAbstractItemModel::endX are thread-affine to the thread of a proxy model. Unfortunately, does not have any backreference to such a proxy model. Either I make beginX/endX affine to the model's own thread or to the UI thread in general. I don't have other options, however, both options lead to unnecessary limitations and possible sideeffects on existing programs. I need more evaluation here to make a final decision.

@mchistovib
Copy link
Author

They've answered me in support, perhaps that would help
image

@omix
Copy link
Contributor

omix commented Dec 21, 2022

I come to the conclusion that simply making AbstractItemModel::beginX and QAbstractItemModel::endX thread-affine is a bad solution. There are different thinkable scenarios of threaded models. As mentioned by Qt support, the only important thing is to call begin and end in the thread of the view. It is even not depending on the model's thread because model and view can basically be owned by different threads. QtJambi's thread check can not cover these situations especially because the model never knows the view(s).
I close this issue with the comment that threaded model/view programming really is advanced stuff. You need to know what you're doing. Qt support gave us a little bit more insight.

@omix omix closed this as completed Dec 21, 2022
@omix
Copy link
Contributor

omix commented Mar 29, 2023

I changed my paradigm and added thread-affinity to model's begin and end methods. They are now affine to the model's thread. It is still possible to prepare tables in different threads as long as the thread owns the table. Finally when bringing into view, the threaded table has to be moved to UI thread. The change will be avaibale with QtJambi version 6.5.0.

omix added a commit that referenced this issue Jul 25, 2023
Bugfix Issue #89
Bugfix Issue #86
Bugfix Issue #85
Bugfix Issue #84
Bugfix Issue #83
Bugfix Issue #79
Bugfix Issue #78
Bugfix Issue #77
Bugfix Issue #75
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants