Stack overflow due to model QProgressDialog::setValue() calls via signal from another thread

oblivioncth

I can try to create a minimal reproducible example if desired, but the issue seems fairly clear.

I moved a large operation to a new thread via the Worker-Object model because of UI responsiveness issues. During the operation a modal QProgressDialog is created because the user isn't allowed to do anything while the procedure is running. Because of my move to a new thread, this dialog is now being updated via a signal/slot connection:

connect(importWorker, &ImportWorker::progressValueChanged, mImportProgressDialog.get(), &QProgressDialog::setValue);

The problem is that while there are many portions of the operation that are long enough to cause the UI to freeze (hence the use of a separate thread), there are some sections where QProgressDialog::setValue() is called rapidly to the point where the previous call to the slot hasn't finished when the next one occurs, with this happening many times in a row to the point where the main/UI thread stack fills and overflows because it never unwinds. Somewhat a reentrancy issue, which is even alluded to in the documentation for setValue().

Stack Overflow

I believe I am directly experiencing what has already been noted here: https://bugreports.qt.io/browse/QTBUG-10561 I do wish that QProgressDialog had another slot (or a configurable option) for updating its value that didnt call QApplication::processEvents() when it is model.

The dialog must be modal and I can't simply futz around to try and have setValue not called to often as there is no deterministic way to do so since its tied to the clients speed and will be affected by any changes I make to the procedure.

I can only see three solutions in absence of the ability to turn off the processEvents() call on a modal QProgressDialog:

1) Make the above connection a blocking queued connection so that the worker thread must wait until setValue() is finished. Not at all ideal due to the slowdown this would cause

2) Subclass QProgressDialog and re-implement setValue(). A lot of work for something trivial and it may not even be possible depending on the access modifiers for QProgressDialog's QProgressBar

3) Make a QProgressDialog from scratch. Even more work for something trivial

4) Use setBar() to replace the QProgressBar with my own that I will control directly instead of using setValue() (and will also handle opening and closing the dialog manually). This solution is awkward but if it doesn't break the behavior of the dialog I want to make use of (hence not wanting to make my own), it could be a decent work around.

It would be much more ideal if I could continue to use QProgressDialog . Is there something loosely similar to QMutex I can use so that the signal/slot connection isn't blocking but that once the loop in the code comes back to where it needs to emit the signal that is connected to QProgressDialog::setValue() it is blocked until the previous call has finished (i.e. locking setValue()?) This is still slower than optimal and the unnecessary QApplication::processEvents() calls are the crux of the issue.

Does anyone know a way to make the dialog effectively modal without truly setting it to be modal, or a better solution then what I could think of?

oblivioncth

Fix for this issue merged and ready for 5.15.2/6.0. :)

https://codereview.qt-project.org/c/qt/qtbase/+/314501

Thanks again for nudging me to do it and helping out SGaist.

SGaist

Hi,

~~4th~~ 5th solution fix QProgressDialog :-)

More seriously, that would be nice but I think number 3 is likely what is going to be the fastest for you currently. QProgressDialog is basically a QDialog with a QProgressBar.

oblivioncth

@SGaist I just edited the OP with a different 4th option that I'd be curious if you have any comments on it.

But man, the call of duty to contribute to the source has found me again lol. I'm trying to get this application out sooner or later, hence why I haven't worked on the issue with QLineEdit yet (if you recall), so I am going to use a temporary solution, especially since even if a change for this is accepted it will be awhile before it comes downstream. Once that happens, I'm going to at least get to the point where I have a change submitted (for this and the QLineEdit issue) even if its not accepted as it would be nice to learn the process.

For this one, what in your opinion is more reasonable, or perhaps more in-line with Qt's design patterns (and therefore likely to be agreed upon and accepted):

A separate slot that never calls processEvents(), similar to how QLineEdit has the distinct textEdited() and textChanged() signals that are largely the same but with one small, but important difference (programmatic vs user only). Not sure what I'd call it though... pushValue()? Idk setValue() being taken is hard to get around.
A boolean argument for setValue that is defaulted to true so that setValue() can still be used, but when setValue(false) is called processEvents() will not be called for QProgressDialogs that are modal.

It will be a bit tricky writing the documentation for the latter since the value will essentially have no effect if the dialog is modaless, so it isn't as simple as saying "true will call processEvents(), false will not", but it seems like the more practical solution to me

SGaist

4 starts to look like you are better off writing your own dialog.

A boolean argument like that is the wrong type. That makes both the documentation and the code hard to read. Either use an enum or maybe suggest an update mode that your can change with a big fat warning.

oblivioncth

@SGaist You may be right about swapping the bar, I'm just trying to get around this quickly for the time being and then will come back to it later with a better solution (or hopefully the fix I submit).

A enum would be clearer on what it does, something like

enum QProgressDialog::UpdateMode {ModalProcessEvents, ModalEventless}
//...
void QProgressDialog::setValue(UpdateMode updateMode = ModalProcessEvents) {...}

with setValue(UpdateMode::ModalEventless) being used in my application for the desire I describe above.

Or perhaps even have the mode set as part of the QProgressBar constructor with a setter available to change it later, as I'm assuming you meant by "update mode". And by warning do you mean in the documentation, like how there are Warning! sections already?

I'm hoping that if my submission ultimately has right approach but there is just disagreement with something like the enum name for example, that it would be mentioned in the rejection votes and it would be fairly trivial to briefly "discuss" the issue within JIRA/Gerrit to reach an agreeable name and then alter the submission accordingly. Just so I don't have to keep bothering you or the form specifically with each shift in the idea. I guess I'll find out one way or another.

SGaist

The code review process might be lengthy at time but you shall not get downvotes without explanations. And a downvote is not the end of the road, it just shows that some corrective work is needed unless you hit an effective roadblock like doing a breaking change. Then again, it's a case where you'll have an explanation why.

oblivioncth

Fix for this issue merged and ready for 5.15.2/6.0. :)

https://codereview.qt-project.org/c/qt/qtbase/+/314501

Thanks again for nudging me to do it and helping out SGaist.

SGaist

Congratulation and thank you for the fix :-)