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] Fix memory leaks in Pet Store client sample #270

Merged

Conversation

etherealjoy
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Allow generated code to do cleanup of allocated memory by preventing premature loop exit.
The generated code has the possibility to delete all memory allocations from itself but the sample client is terminating the loop before allowing the callback to return and memory de-allocation to take place.
With this change the callback from the timer and response schedule an event to the eventloop and then perform the clean up after which the exit is done.

Tests

  • Running valgrind on the PetStore Client sample binary produces no warning of memory leak.

@stkrwork @MartinDelille @ravinikam @fvarose

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Jun 10, 2018

@MartinDelille
You had opened an issue to modernize the Qt sample, As of now no memory leaks are in the generated code.

However the setter are still not user friendly in that for pointer types the user has to care about de-allocating.
We can make it such that the setters themselves update the data structures internally instead of replacing the pointers and the user has to take care of his/her own allocated memory.
for example,
setters for string members, or list members which are using pointers to memory for data storage.

@MartinDelille
Copy link
Contributor

I quickly review your PR but I don’t understand why you’re not using local variables instead of pointer: you wouldn’t have to deallocate the memory at the end of the block and would reduce the code size.

I will point an example in your code and make a proposal.

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Jun 10, 2018

@MartinDelille
Indeed, I did not add any new variables just refactor old ones. But if you think we should I can do this here itself.
Please add the example also would be great.

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Jun 12, 2018

@MartinDelille @stkrwork
Should I refactor here or just fix current ?
I would prefer to just fix current here and refactor with another issue.

@MartinDelille
Copy link
Contributor

@ethereajoy ok do as you wish!


QList<QString*>* status = new QList<QString*>();
status->append(new QString("available"));
status->append(new QString("sold"));
auto available = new QString("available");
Copy link
Contributor

Choose a reason for hiding this comment

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

What about:

const QString available = “available “

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will do it in the next update.
I plan to switch to QVariant for all members and lets see. It will be a breaking change but I will make it with the server first.

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Jun 17, 2018

OK, I will create a different issue to track these for both Qt5 Client and Server since they share common code.

@etherealjoy etherealjoy reopened this Jul 2, 2018
@etherealjoy
Copy link
Contributor Author

@wing328
I think we can go ahead with this.

@wing328
Copy link
Member

wing328 commented Jul 8, 2018

The CircleCI (1.0) error can be ignored as latest master has migrated to use CircleCI 2.0 with better and more stable build.

@wing328 wing328 added this to the 3.1.1 milestone Jul 8, 2018
@wing328 wing328 merged commit 6d05ea5 into OpenAPITools:master Jul 8, 2018
@etherealjoy etherealjoy deleted the cppqt5_remove_memory_leaks branch July 8, 2018 07:59
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
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

4 participants