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

lwipClient - fix mem pool leak #207

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JAndrassy
Copy link
Contributor

fix mem pool leak in lwipClient with shared_ptr with custom deleter.
for lwipClient created from lwipServer the deleter doesn't free the struct. server releases the clients it manages. it would be better if the server used an array of lwipClient objects. then the shared_ptr in them would hold the pointer. maybe in a follow up PR if this is merged.

@per1234 per1234 self-assigned this Dec 14, 2023
@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Dec 14, 2023
@aentinger
Copy link
Contributor

aentinger commented Dec 14, 2023

I'm not seeing it. Sure using a std::shared_ptr would be a pretty clean solution, but you could always just call delete (or mem_free) in the Destructor. Having a mem_alloc (a C function) inside of reset (a member of a C++ template class) just feels super weird. What do you think?

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Dec 14, 2023

you could always just call delete (or mem_free) in the Destructor.

no. Client must be copyable. multiple copies of Client can point to the same TCP connection

having a mem_alloc (a C function) inside of reset (a member of a C++ template class) just feels super weird

not really. the resetfunction is just a assignment which allows to specify a deleter. uses of shared_ptr should be written as close as possible as with a basic pointer

@aentinger
Copy link
Contributor

Client must be copyable

Overload copy-ctor. But yeah, I do see your point somewhat. Can you prepare a adjacent PR showing how you'd like to take this further?

@JAndrassy
Copy link
Contributor Author

Can you prepare a adjacent PR showing how you'd like to take this further?

ok

@andreagilardoni
Copy link
Contributor

This issue depends on arduino/ArduinoCore-API#218

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Jan 3, 2024

This issue depends on arduino/ArduinoCore-API#218

no. tcp_client is not a class
and WiFiClient is never deleted from LwipClent pointer and it even doesn't make sense to do new for WiFiClient. that is why Arduino is not really missing the virtual destructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants