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

[TW#13918] lwip/netconn: Memory leak for netconn_delete() #784

Closed
devsaurus opened this issue Jul 9, 2017 · 16 comments
Closed

[TW#13918] lwip/netconn: Memory leak for netconn_delete() #784

devsaurus opened this issue Jul 9, 2017 · 16 comments

Comments

@devsaurus
Copy link
Contributor

devsaurus commented Jul 9, 2017

I'm evaluating the lwip netconn API and investigate some memory leaks with closed connections.
My application code assumes that a call to netconn_delete() finally frees all resources, but I observe that this is not true. I need to remove the #if !ESP_THREAD_SAFE in line 176 to get heap to return to initial level. Otherwise, each accept/send/close/delete cycle on a server netconn leaks ~70 bytes of heap memory. Similar for client connections.

What was the rationale to disable netconn_free() in case of ESP_THREAD_SAFE configuration? If it's required, how to fix the memory leak when netconn_free() is not executed - Is there a thread-safe method to free it?

@negativekelvin
Copy link
Contributor

negativekelvin commented Jul 9, 2017

I believe if you are using netconn api you have to handle thread safety on your own, so you decide when to call netconn_free and call it yourself. Otherwise, use socket api.

@hwmaier
Copy link
Contributor

hwmaier commented Jul 10, 2017

This sounds familiar to the issue I mentioned here #537.

Make sure you do not use lwip_... calls as they are not automatically mapped to the thread safe counterparts.

@projectgus
Copy link
Contributor

projectgus commented Jul 10, 2017

Hi @devsaurus ,

The thread-safe socket layer made this change in order to support closing a BSD socket from one thread while a pending read/write was running in another. This behaviour change for netconn_close() API looks like an unintended consequence.

Until we have a fix, the best workaround is probably to call netconn_free() after netconn_close().

Angus

@devsaurus
Copy link
Contributor Author

Thanks for the feedback, guys!

I exclusively use netconn_ calls so the handover between my single-task application (the NodeMCU Lua firmware) and the lwip stack should be safe. I assume that this is also true for calling netconn_free() after closing as Angus proposed.

Somewhat off-topic: I found that I strictly have to avoid calling netconn_ functions from the application callback in tcpip-task context (registered with netconn_new_with_callback()). This results in almost immediate stalls with I (81906) wifi: ap_probe_send over, resett wifi status to disassoc symptom.

@FayeY FayeY changed the title lwip/netconn: Memory leak for netconn_delete() [TW#13918] lwip/netconn: Memory leak for netconn_delete() Jul 12, 2017
@tonyp7
Copy link

tonyp7 commented Nov 7, 2017

I was getting crazy with this memory leak. Thank you for pointing me to this line 176!

tonyp7 added a commit to tonyp7/esp32-wifi-manager that referenced this issue Nov 7, 2017
@Ritesh236
Copy link

Hi,

We have also faced same memory leak issue while using netconn library at our side. so, what is the workaround or final fix for that issue?

We are using netconn open/close/accept/listen all those APIs into our project development purpose.

Please help us or any suggestions for that solution.

@tonyp7
Copy link

tonyp7 commented Nov 25, 2017

Add a call to netconn_free after netconn_delete.

@Ritesh236
Copy link

Ok. Let me check and get back to you with updated result.

@zhangyanjiaoesp
Copy link
Collaborator

zhangyanjiaoesp commented Dec 4, 2017

@devsaurus @tonyp7 @Ritesh236 Sorry to reply so late, we have modified the code against your problem. After the MR is merged, you can see the change

@Ritesh236
Copy link

Hi, Thanks for Reply.

So, Will that fix be included into ESP32 LWIP Library in which netconn APIs are defined?

@zhangyanjiaoesp
Copy link
Collaborator

yes, it will be

@igrr igrr closed this as completed in 210d349 Dec 5, 2017
@Ritesh236
Copy link

Hi, Thanks for providing update. It seems like you have fixed into ESP32 IDF on Git. Correct?

I will check it and let you know if any issue.

@Ritesh236
Copy link

Hi,

Also just want to confirm that now we don't need to call netconn_free after netconn_delete API as it has been fixed into LWIP code itself.

Correct?

@zhangyanjiaoesp
Copy link
Collaborator

@Ritesh236 yes, you are right

igrr pushed a commit that referenced this issue Dec 7, 2017
tonyp7 added a commit to tonyp7/esp32-wifi-manager that referenced this issue Dec 10, 2017
no longer necessary to call netcon_free since the memory leak was fixed
with latest version of esp-idf
@Designer-Rick
Copy link

Well spotted on line 176!!! that was driving me crazy

@zhangyanjiaoesp
Copy link
Collaborator

@Designer-Rick Did you test with the latest esp-idf ? Please post your commit ID and your error log, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants