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

[cpp][Qt5] Add the ability to pass QNetworkAccessManager as a parameter #6053

Merged
merged 4 commits into from
May 7, 2020

Conversation

alfredotg
Copy link
Contributor

Problem:
Every api call created new connection to server. It happens because OAIHttpRequestWorker creates new QNetworkAccessManager every time.

Solution:
Passing QNetworkAccessManager as parameter. It allows QT reusing an opened early connection if it possible (if server support keep alive connections and if there is no active request).
This change is optional and should not affect existing code.
Also I change connect to signal QNetworkReply::finished instead of QNetworkAccessManager::finished.

@ravinikam @stkrwork @etherealjoy @MartinDelille @muttleyxd

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./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).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

alfredotg added 2 commits April 25, 2020 12:38
* connect to QNetworkReply::finished instead of QNetworkAccessManager::finished
Copy link
Contributor

@etherealjoy etherealjoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void {{prefix}}HttpRequestWorker::on_manager_timeout(QNetworkReply *reply)

Need to disconnect in the function from the reply

@etherealjoy
Copy link
Contributor

@alfredotg
Many thanks for the PR,
I am wondering about the chance of a crash when the api object is deleted but the reply object is still alive because now it is managed by the manager and it could still call the slot.

@alfredotg
Copy link
Contributor Author

alfredotg commented Apr 26, 2020

@etherealjoy

@alfredotg
Many thanks for the PR,
I am wondering about the chance of a crash when the api object is deleted but the reply object is still alive because now it is managed by the manager and it could still call the slot.

Reply object is child of OAIHttpRequestWorker so it will be deleted with OAIHttpRequestWorker which will be deleted along with api object.

void {{prefix}}HttpRequestWorker::on_manager_timeout(QNetworkReply *reply)

Need to disconnect in the function from the *reply

Thanks. I will fix it.

@MartinDelille
Copy link
Contributor

@etherealjoy I think we could improve the code memory management thanks to QSharedPointer and friends.

@alfredotg
Copy link
Contributor Author

@etherealjoy I think we could improve the code memory management thanks to QSharedPointer and friends.

Why and where we need shared point? I thought parent-child relationships were the main way to manage memory in Qt.

@etherealjoy etherealjoy merged commit 3bfd6de into OpenAPITools:master May 7, 2020
jimschubert added a commit that referenced this pull request May 8, 2020
…-5.0

* origin/master: (78 commits)
  [powershell-experimental] : http signature authentication implementation (#6176)
  Add missing AnyType type mapping (#6196)
  remove pubspec.lock (#6208)
  Minor fixes post-release (#6204)
  [cpp][Qt5] Add the ability to pass QNetworkAccessManager as a parameter  (#6053)
  comment out dart2 test due to failure
  adds the missing typeMapping for AnyType (#6199)
  [Rust Server] Support boolean headers, and fix panic handling headers (#6056)
  update samples
  update 5.0.0 release date
  update readme with new release
  Prepare 4.3.1 release (#6187)
  Fix #6157: Updated native template to fix null async return (#6168)
  Show description when summary is missing (#6159)
  Make the array items optional (#6132)
  [aspnetcore] test petstore samples in drone.io (#6148)
  fix bearer auth in c# netcore async call (#6136)
  skip web.config for aspnetcore 3.x (#6147)
  Adds memoization and deserialization through 2 or more discriminators (#6124)
  Implement Asp.Net Core 3.0/3.1 generator (#6009) (#6025)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants