-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Simpler timeout with QTimer::singleShot #4435
Simpler timeout with QTimer::singleShot #4435
Conversation
This mistake leverage a lack in our testing: the CI should test that client has been updated. |
https://doc.qt.io/qt-5/qobject.html#disconnect
So indeed original code below is correct. void PFXHttpRequestWorker::on_manager_timeout(QNetworkReply *reply) {
error_type = QNetworkReply::TimeoutError;
response = "";
error_str = "Timed out waiting for response";
disconnect(manager, nullptr, nullptr, nullptr);
reply->abort(); I was trying to suggest this below, but since the code was reworked in your PR I got confused. void PFXHttpRequestWorker::on_manager_finished(QNetworkReply *reply) {
if(timer->isActive()){
timer->stop();
}
disconnect(timer, nullptr, nullptr, nullptr);
error_type = reply->error();
response = reply->readAll();
error_str = reply->errorString(); The idea is that, once we received the response callback, we should deactivate timer to avoid the timeout from triggering. |
Do we really need to disconnect in the end? |
Yes we do. |
So we dont get both the callbacks for each API call. |
Ok so I push the new fix |
This should solve one topic related to disconnect response callback when timeout. |
There is another bug in this branch.
self assignment. |
But the worker is deleted later in the callback. Shouldn't it disconnect from the futur timer event? (https://doc.qt.io/qt-5/qobject.html#deleteLater)
My bad! 😅 |
I think it is OK.
Lets see if there are issues reported later. |
Ok I pushed the self assignement fix. I think this should be the worker responsability to delete itself later (and not the callback). |
@MartinDelille thanks for the PR, which has been included in the v4.2.1 release: https://twitter.com/oas_generator/status/1195339336922759168 |
This fix the problem mentionned here: #4430 (comment)
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.@ravinikam @stkrwork @etherealjoy @muttleyxd