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

Added close_abort() function to WiFiClient #2767

Closed
wants to merge 10 commits into from

Conversation

pfabri
Copy link

@pfabri pfabri commented Dec 15, 2016

This branch adds a close_abort() method to the WiFiClient class.

How it works, what it does:

Calling close_abort() will close and abort the client connection it
is invoked on and, as a result, free its resources (i.e. memory).

WARNING: aborting connections without a good reason violates the
TCP protocol, because a closed connection would normally need to
spend some time in TIME_WAIT state before its resources are freed.

Usage example:

WiFiClient client;		// set up a client

{ /* do things with your client */ }

client.stop_abort()		// when you're done,abort the
			// connection if you must

Why it's useful:

  1. Give programmers a way to shut down connections immediately if
    need be. The underlying tcp.c file has an abort function, but
    this has not been directly accessible via the WiFiClient
    class until now.

  2. There are a number of reported issues for the repository
    addressing the heap corruption that can result from trying to
    retain too many connections in TIME_WAIT state (most notably:
    Memory leak ESP8266WebServer #230, memory leak by using WiFiClient (40 Byte per usage) #1070, memory leak 184 bytes at a time #1923). Although the warning above holds, there may be
    circumstances where this isn't very important. For example an ESP8266
    running in AP mode hosting a page, which requests a new
    connection every second via an AJAX script to monitor
    a sensor/button/etc. continusously.

Currently existing alternative approach:

When building a project, defining the
-D MEMP_NUM_TCP_PCB_TIME_WAIT=5compiler directive will limit the
maximum number of clients allowed to stay in TIME_WAIT state. 5 is
the default, lower it as necessary. See reference
here

Thanks:

Thank you to @me-no-dev, @everslick and @Palatis for bringing the MEMP_NUM_TCP_PCB_TIME_WAIT option to my attention.

This branch adds a close_abort() method to the WiFiClient class.

How it works, what it does:
Calling `close_abort()` will close and abort the client connection it
is invoked on and, as a result, free its resources (i.e. memory).

**WARNING:** aborting connections without a good reason violates the
TCP protocol, because a closed connection would normally need to
spend some time in `TIME_WAIT` state before its resources are freed.

Usage example:
    WiFiClient client;		// set up a client

    { /* do things with your client */ }

    client.stop_abort()		// when you're done,abort the
				// connection if you must

Why it's useful:
1. Give programmers a way to shut down connections immediately if
   need be. The underlying `tcp.c` file has an abort function, but
   this has not been directly accessible via the `WiFiClient`
   class until now.

2. There are a number of reported issues for the repository
   addressing the heap corruption that can result from trying to
   retain too many connections in `TIME_WAIT` state (most notably:
   esp8266#230, esp8266#1070, esp8266#1923). Although the warning above holds, there may be
   circumstances where this isn't very important. For example an ESP8266
   running in AP mode hosting a page, which requests a new
   connection every second via an AJAX script to  monitor
   a sensor/button/etc. continusously.

Currently existing alternative approach:
When building a project, defining the
`-D MEMP_NUM_TCP_PCB_TIME_WAIT=5`compiler directive will limit the
maximum number of clients allowed to stay in TIME_WAIT state. `5` is
the default, lower it as necessary. See reference
[here](https://github.com/esp8266/Arduino/blob/master/tools/sdk/lwip/include/lwipopts.h#L263)

Thanks:
Thank you to @me-no-dev, @everslick and @Palatis for bringing the `
MEMP_NUM_TCP_PCB_TIME_WAIT` option to my attention.
@codecov-io
Copy link

Current coverage is 27.80% (diff: 100%)

Merging #2767 into master will not change coverage

@@             master      #2767   diff @@
==========================================
  Files            20         20          
  Lines          3625       3625          
  Methods         335        335          
  Messages          0          0          
  Branches        656        656          
==========================================
  Hits           1008       1008          
  Misses         2441       2441          
  Partials        176        176          

Powered by Codecov. Last update 7b32e6a...9fac290

@psy0rz
Copy link

psy0rz commented May 1, 2017

We at the ESPEasy project need this as well: Sometimes we need to make a lot of new connections in a short time, causing memory problems.

Cant seem to find a workaround that does not involve patching Wificlient itself.

@kevinkk525
Copy link

kevinkk525 commented Jul 13, 2017

Thanks a lot! This solved my issue after hours of searching for the memory leak in my server application.. lost 1.6kB of heap within 10 messages with different clients.
Why is this commit still not merged into the main branch?
I somehow doubt that this problem is so minor that it can be ignored. or is my server software doing something wrong by not closing the connection or similar?

@shimizutko if you are still interested in it: I just added the changes to those files regardless of any other differences. This worked perfectly. And I'm calling stop_abort instead of stop of course.

@psy0rz
Copy link

psy0rz commented Jul 14, 2017

a cleaner workaround that doesnt require changes to core libraries: #1923 (comment)

@kevinkk525
Copy link

Thanks for your answer! Funny thing is, I also found that, but it does not make any difference at all. I put this code before any includes except Arduino.h as I get an error if I put it before Arduino.h or use my code without Arduino.h.
It just does not make any difference for me. If it did, it would be the better workaround of course.

@everslick
Copy link
Contributor

everslick commented Jul 14, 2017 via email

@kevinkk525
Copy link

Thanks a lot! This does work quite well! Forgive me, I somehow forgot to call that function..
Thank you for all the help!

@psy0rz
Copy link

psy0rz commented Jul 14, 2017

yeah you have to call it at a point you think there is some cleaning up to do. or just in loop(). no harm in calling it too often i think.

@earlephilhower
Copy link
Collaborator

@d-a-v, I think this is no longer needed with your TIME_WAIT fixes. If so, can you please close?

@earlephilhower earlephilhower added this to the 2.6.0 milestone Sep 29, 2019
@earlephilhower earlephilhower requested a review from d-a-v October 1, 2019 19:49
@earlephilhower earlephilhower added merge-conflict PR has a merge conflict that needs manual correction waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. labels Oct 1, 2019
@earlephilhower
Copy link
Collaborator

Thanks for your PR, but the core and libraries have changed enough that this PR now has a merge conflict.

Could you merge it manually with the latest core, so we can consider it for future releases?

@devyte devyte modified the milestones: 2.6.0, 2.7.0 Oct 29, 2019
@devyte
Copy link
Collaborator

devyte commented Oct 29, 2019

This has a conflict, pushing back.

@d-a-v d-a-v removed merge-conflict PR has a merge conflict that needs manual correction waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. labels Jan 8, 2020
@d-a-v d-a-v removed this from the 2.7.0 milestone Feb 21, 2020
@d-a-v d-a-v added this to the 3.0.0 milestone Feb 21, 2020
@earlephilhower earlephilhower added the merge-conflict PR has a merge conflict that needs manual correction label Feb 27, 2020
@d-a-v d-a-v removed the merge-conflict PR has a merge conflict that needs manual correction label Nov 9, 2020
@d-a-v
Copy link
Collaborator

d-a-v commented Nov 9, 2020

I think this is no longer needed with your TIME_WAIT fixes. If so, can you please close?

The primary goal of this PR has indeed been fixed by @me-no-dev's patch that is ported to lwip2.
If such an API is not needed for other purpose we can close this PR.

@d-a-v d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Nov 9, 2020
@d-a-v
Copy link
Collaborator

d-a-v commented Feb 28, 2021

Without feedback and due to old age, also with another fix addressing the original issue that is already in the core, this PR should be closed.
It can be reopened after discussion, if any.

@d-a-v d-a-v closed this Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants