qtjambi icon indicating copy to clipboard operation
qtjambi copied to clipboard

QML thread affinity check is missing

Open mchistovib opened this issue 3 years ago • 4 comments

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 avatar Sep 28 '22 12:09 mchistovib

@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

mchistovib avatar Sep 29 '22 11:09 mchistovib

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.

omix avatar Sep 29 '22 14:09 omix

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

mchistovib avatar Sep 29 '22 17:09 mchistovib

@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 avatar Sep 30 '22 10:09 omix

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

omix avatar Oct 24 '22 19:10 omix

@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.

mchistovib avatar Oct 25 '22 12:10 mchistovib

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.

omix avatar Oct 25 '22 13:10 omix

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

mchistovib avatar Oct 26 '22 13:10 mchistovib

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 avatar Dec 21 '22 15:12 omix

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 avatar Mar 29 '23 12:03 omix