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

stopAll() in WiFiGeneric causes currently in progress webServer transmission to stop. #6123

Closed
PurpleAir opened this issue May 20, 2019 · 5 comments · Fixed by #8598
Closed

Comments

@PurpleAir
Copy link
Contributor

This line:

causes a currently in session webServer send to stop. Before I make an MCVE, can anyone share why this stopAll exists?

If, while the web server is sending a chunked response, a WiFi event occurs, the chunked response is abandoned and the web browser gives an error. Commenting it out appears to fix the problem but maybe there was a reason for it and something else would be broken after removing it?

It may also have some bearing on an older issue: #1554

@TD-er
Copy link
Contributor

TD-er commented May 20, 2019

I can imagine it would be useful to enter sleep mode as soon as possible and leave in a known state.

@d-a-v
Copy link
Collaborator

d-a-v commented May 20, 2019

@adrionics This is history and legacy.
I guess every tcp connection should not be stopped if AP is active with ongoing tcp connections.
Also if STA is reconnected soon enough, ongoing connections should be able to go on.
Is that what you want to show with an MCVE ?

I currently can't think of anything that would prevent the removal of stopAll(). Ongoing connections would just timeout or have connected() set to false (this has to be verified).

I can imagine it would be useful to enter sleep mode as soon as possible and leave in a known state.

Agreed, then stopAll() would need to be explicitly called at that time

@PurpleAir
Copy link
Contributor Author

My feeling is stopAll() should always be user initiated and if you are going to sleep, call it if you need to.

My issue was happening in AP_STA mode, while not connected to WiFi as a STA, but with a client connected to the AP and downloading a web page from the ESP. The AP client stays connected, just the web browsers TCP connection is interrupted when the ESP attempts to connect to a non existent network and that fails. Looking at the code, any WiFi event would cause this though.

Do you want an MCVE?

@devyte
Copy link
Collaborator

devyte commented May 21, 2019

@adrionics the method must be called if an interface goes down, e. g. the STA disconnects, for cleanup purposes. In that case, it is up to the user app layer to restart services, usually by implementing the restart in a wifi event callback.
If this happens in the middle of sending/receiving, it is up to the app layer to handle that. There are no low level retries or continues for a communication that gets broken in such a way.

@d-a-v I do believe we currently have a single list of sockets for both AP and STA. We could split the list, but we would need to have one per interface (e. g. loopback, Ethernet_lwip, etc). We could then close only those relevant to the interface that went down.

@devyte devyte added this to the 3.0.0 milestone Nov 20, 2019
@devyte devyte assigned devyte and unassigned devyte Nov 20, 2019
@d-a-v
Copy link
Collaborator

d-a-v commented Mar 6, 2021

We could then close only those relevant to the interface that went down.

TCP connections must be closed before the interface goes down.
We don't need to have separate lists for AP and STA. What would be needed is to check for which interface each connection of the unique linked list is made and close if that interface is supposed to be shut down.

The question arisen in this issue is to remove core-initiated calls to WiFiClient::stopAll() and let user deal with them (the opened connections). I think that's still relevant.

@d-a-v d-a-v self-assigned this Mar 6, 2021
@d-a-v d-a-v modified the milestones: 3.0.0, 3.0.1 Mar 31, 2021
@d-a-v d-a-v modified the milestones: 3.0.1, 3.1 Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants