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

AT server.available fix #20

Merged
merged 1 commit into from Sep 22, 2023
Merged

Conversation

JAndrassy
Copy link
Contributor

  • don't return clients of other servers. this was a major error
  • only return clients with data available. it is how server.available should work
  • accept all waiting clients (up to max), because not all of them may have data available and we may want to server.print to all

Copy link
Contributor

@maidnl maidnl left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution.
Removing the break at line 492 is good although this was not preventing the connection of new clients: since server.available() must be called repeatedly all clients will be eventually accepted once at time (the removal of the break allows to accept more than one at once though) .
At line 501 you spotted a real major problem: returning the client without checking if it belong to the current Server is clearly a bug, without the check serverClients[last_server_client_sock].server == sock we may return a wrong client (although this would happen only in case of more than one Server instance).
My main concern is related to the line && serverClients[last_server_client_sock].client.available() > 0, I get the spirit and the purpose of this check and generally speaking there is nothing wrong with it. But what happen if (as it usually is) the connect client ask for something and then it stop making other requests? If that happen there won't be anymore available data from that client, the client won't be returned anymore but it will keep to occupy a position into the serverClient array preventing the possibility for new clients to connect.
If you want to have this possibility this means that you have to do something at application level: for example you need to keep track of all the connected clients and having some mechanism to close the ones you are not interested anymore (for example you would drop all clients that are not sending any request for more than a certain time): this is fine but, in my opinion, not in in line with the spirit of Arduino to keep things simple as much as possible for the final user.
In standard Arduino Server example, the Server always close the connection with client as soon as it serves the request from that client. This prevent the filling of the serverClient array allowing new connections to be handled.
The server.available() must always return the last connected client for that Server, until the client issues is request, then the server handle it and close the connection.
This keeps things simple and does not lead to the problem of the exhaustion of the serverClient array, allowing multiple client to connect with the server.

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Sep 18, 2023

@maidnl please read
https://www.arduino.cc/reference/en/libraries/wifinina/server.available/

this way it is in Ethernet library and all WiFi libraries

the old WiFiChatServer and my new PagerServer example demonstrate this

@maidnl
Copy link
Contributor

maidnl commented Sep 18, 2023

I am sorry but I can't see any contradiction between the current implementation and the documentation you quoted.

@JAndrassy
Copy link
Contributor Author

JAndrassy commented Sep 18, 2023

Gets a client that is connected to the server and has data available for reading.

It is a logical "and" in all implementations of server.available in other Arduino WiFi libraries and in the Ethernet library.
If a client is returned from server.available() it must have data to read. client.available() > 0 must be true

- don't return clients of other servers
    this was a major error
- only return clients with data available
    it is how server.available should work
- accept all waiting clients (up to max)
    because not all of them may have data available
    and we may want to server.print to all
@facchinm facchinm merged commit ec62308 into arduino:main Sep 22, 2023
1 check passed
@JAndrassy JAndrassy deleted the server_available_fix branch September 22, 2023 18:26
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.

None yet

3 participants