Skip to content
This repository has been archived by the owner on Jan 20, 2025. It is now read-only.

Collected other pull request and Fixbug memory leaks and weakness argument pointer for tcp/ip callback function #173

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

TienHuyIoT
Copy link

Fixbug memory leaks

  • AysncClient recv callback handle missing call pbuf_free()

Fix crash tcp/ip api calls

  • Due to tcp_api_call_t msg declared local variant and used to as a pointer argument for the callback function. So this does not make sure that the memory holding the parameters of the pointer variant is handled right.

@mathieucarbou
Copy link
Contributor

This project seems quite dead since 4y... I would rather send PRs against this one now: https://github.com/esphome/AsyncTCP which is maintained by ESPHome (https://esphome.io)

@TienHuyIoT
Copy link
Author

This project seems quite dead since 4y... I would rather send PRs against this one now: https://github.com/esphome/AsyncTCP which is maintained by ESPHome (https://esphome.io)

Thanks,
You can reference my pull request. It has some fix-bug relevant to memory leaks and handling weakness tcp/ip callback function.
I checked your repository. It still has these bug in your repo.

@mathieucarbou
Copy link
Contributor

mathieucarbou commented Nov 2, 2023

This project seems quite dead since 4y... I would rather send PRs against this one now: https://github.com/esphome/AsyncTCP which is maintained by ESPHome (https://esphome.io)

Thanks, You can reference my pull request. It has some fix-bug relevant to memory leaks and handling weakness tcp/ip callback function. I checked your repository. It still has these bug in your repo.

This is not my repo ;-)
I am just watching and saw your PR, that's it.

If you leave it there, it will be lost. Nobody will act on it.
If you send it against a repo maintained by an organisation like ESPHome, more likely somebody will look at it and merge it.

But I have no relationship with any of these projects ;-)
Like said, I was just noticed about your PR through email and I think it has a lot of value so I took time to comment and advise you to instead send it against a maintained repo, not a dead one ;-)

@TienHuyIoT
Copy link
Author

TienHuyIoT commented Nov 2, 2023

This project seems quite dead since 4y... I would rather send PRs against this one now: https://github.com/esphome/AsyncTCP which is maintained by ESPHome (https://esphome.io)

Thanks, You can reference my pull request. It has some fix-bug relevant to memory leaks and handling weakness tcp/ip callback function. I checked your repository. It still has these bug in your repo.

This is not my repo ;-) I am just watching and saw your PR, that's it.

If you leave it there, it will be lost. Nobody will act on it. If you send it against a repo maintained by an organisation like ESPHome, more likely somebody will look at it and merge it.

But I have no relationship with any of these projects ;-) Like said, I was just noticed about your PR through email and I think it has a lot of value so I took time to comment and advise you to instead send it against a maintained repo, not a dead one ;-)

I got it,
Thank you!

@TienHuyIoT TienHuyIoT force-pushed the master branch 2 times, most recently from ee0155f to fd6447f Compare November 4, 2023 03:30
- Running async tcp by default core
- Disable watchdog handler async tcp task
- _tcp_recved() should be called when it has processed the data
@@ -417,7 +422,7 @@ static esp_err_t _tcp_write(tcp_pcb * pcb, int8_t closed_slot, const char* data,
static err_t _tcp_recved_api(struct tcpip_api_call_data *api_call_msg){
tcp_api_call_t * msg = (tcp_api_call_t *)api_call_msg;
msg->err = ERR_CONN;
if(msg->closed_slot == -1 || !_closed_slots[msg->closed_slot]) {
if(msg->closed_slot != -1 || !_closed_slots[msg->closed_slot]) {
Copy link
Contributor

@mathieucarbou mathieucarbou Apr 26, 2024

Choose a reason for hiding this comment

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

this condition does not make any sense anymore: is it tested ? If msg->closed_slot == -1, second expression is evaluated with an index being -1. If not -1, second expression is not evaluated.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TienHuyIoT : ^^ ?

Copy link
Contributor

Choose a reason for hiding this comment

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

closed_slot is int8_t so -1 means 255, but _closed_slots is an array of 16.

mathieucarbou added a commit to mathieucarbou/AsyncTCP that referenced this pull request Apr 27, 2024
mathieucarbou added a commit to mathieucarbou/AsyncTCP that referenced this pull request Apr 27, 2024
mathieucarbou added a commit to mathieucarbou/AsyncTCP that referenced this pull request Apr 27, 2024
@mathieucarbou
Copy link
Contributor

I've grabbed most of the fixes in my fork: mathieucarbou/AsyncTCP#10

Still, please see my comment. A change in one condition looks really weird to me.

- Increase queue to 128
Comment on lines +1042 to +1044
while (connected() && !send()) {
taskYIELD();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@TienHuyIoT: when using SSE events in ESPAsycnWebServer, I think this busy loop can wait forever in case of a WIFi disconnection because the client is still seen there and connected

}
pbuf_free(b);
Copy link
Contributor

Choose a reason for hiding this comment

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

The pbuf_free change is OK but the remaining changes no.
See here for more details: tbnobody/OpenDTU#2326 (comment)

This causes a drastic slowdown of the readings.

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

Successfully merging this pull request may close these issues.

3 participants