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

Add function so that the total number of web-socket clients can be limited #591

Merged
merged 23 commits into from
Sep 24, 2019

Conversation

matt123p
Copy link
Contributor

@matt123p matt123p commented Sep 9, 2019

This is to cope with a problem when a browser does not close the web-socket connection correctly.

I have observed this on Chromium based browsers. The resource leak will eventually lead to the server crashing with random heap errors (after around 13 or so client connections).

Normally only one connection per client is required, so limiting the number of connections would not normally cause any problems.

The new function checks to see if the maximum number concurrent connections is exceeded and then closes the oldest connection. If this connection was required then the client will normally just reconnect.

It should be called from with in the main loop periodically. In my application I call it around once per second and this seems to work fine. A really obnoxious client could crash your server by opening connections faster but it seems unlikely that would be a problem in most real-world applications.

…mited easily. This is to cope with a problem when a browser does not close the web-socket connection correctly. I have observed this on Chromium based browsers. The memory leak will eventually lead to the server crashing. Normally only one connection per client is required, so limiting the number of connections would not normally cause any problems.
Frequently when using Web Sockets you will get the assert failure:
    	assertion "new_rcv_ann_wnd <= 0xffff" failed: file "/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/lwip/lwip/src/core/tcp.c", line 779, function: tcp_update_rcv_ann_wnd

This will happen particulary when you close the browser window.  This change
prevents the issue from occuring.
…placeholder class that can be used to implement locking at a later date.
…placeholder class that can be used to implement locking at a later date.
…mited easily. This is to cope with a problem when a browser does not close the web-socket connection correctly. I have observed this on Chromium based browsers. The memory leak will eventually lead to the server crashing. Normally only one connection per client is required, so limiting the number of connections would not normally cause any problems.
# Conflicts:
#	src/AsyncWebSocket.h
@me-no-dev
Copy link
Owner

Do you mind adding this to the examples as well? At least to the ones using WS. Also will be good to add a note in the README :)

@matt123p
Copy link
Contributor Author

OK. Stupid error. I have fixed the build.

@@ -255,6 +255,7 @@ class AsyncWebSocket: public AsyncWebHandler {

void close(uint32_t id, uint16_t code=0, const char * message=NULL);
void closeAll(uint16_t code=0, const char * message=NULL);
void cleanupClients(uint16_t maxClients = 4);
Copy link
Owner

Choose a reason for hiding this comment

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

this number should really equal the max open TCP connections of the device. for ESP8266 that is 4 or 5 and for ESP32 that is 16. Having it 4 by default will make it limited on ESP32

@matt123p
Copy link
Contributor Author

Remember this is is the number of web socket connections, not total TCP connections. Typically you will only want one connection per client and you will want to keep some TCP slots available for the other HTTP connections.

Setting it to 4 means that the server will support a maximum of 4 simultaneous clients. I found when the number of connections reaches around 12, then the ESP32 crashes - probably due to the fact there are still some connections open for the other HTTP fetches for the page. If you let it go to the maximum of 16 connections then no other client will be able to connect to the server.

@me-no-dev
Copy link
Owner

4 seems low though. let's agree on higher value for esp32

@matt123p
Copy link
Contributor Author

How about 8? This is only the default the user can override it by passing a parameter.

@me-no-dev
Copy link
Owner

8 sounds better :)

@me-no-dev me-no-dev merged commit 2f37037 into me-no-dev:master Sep 24, 2019
@matt123p matt123p deleted the websockets-limit-clients branch September 25, 2019 06:43
DanielKnoop pushed a commit to DanielKnoop/ESPAsyncWebServer that referenced this pull request Jun 17, 2022
…mited (me-no-dev#591)

* Add function so that the total number of web-socket clients can be limited easily.  This is to cope with a problem when a browser does not close the web-socket connection correctly.  I have observed this on Chromium based browsers.  The memory leak will eventually lead to the server crashing.  Normally only one connection per client is required, so limiting the number of connections would not normally cause any problems.

* Prevent an assertion failure when using WebSockets

Frequently when using Web Sockets you will get the assert failure:
    	assertion "new_rcv_ann_wnd <= 0xffff" failed: file "/Users/ficeto/Desktop/ESP32/ESP32/esp-idf-public/components/lwip/lwip/src/core/tcp.c", line 779, function: tcp_update_rcv_ann_wnd

This will happen particulary when you close the browser window.  This change
prevents the issue from occuring.

* Do not use thread locking with the ESP8266, but instead use an empty placeholder class that can be used to implement locking at a later date.

* Do not use thread locking with the ESP8266, but instead use an empty placeholder class that can be used to implement locking at a later date.

* Add function so that the total number of web-socket clients can be limited easily.  This is to cope with a problem when a browser does not close the web-socket connection correctly.  I have observed this on Chromium based browsers.  The memory leak will eventually lead to the server crashing.  Normally only one connection per client is required, so limiting the number of connections would not normally cause any problems.

* Set the default number of ws clients dependent on processor.
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

Successfully merging this pull request may close these issues.

2 participants