Open
Description
Could you please make a method to actually set timeout in WiFiClientSecureCtx?
You are hard coding the timeout, which overrides the setTimeout method, so in this case setTimeout has no effect.
It sometimes overrides with 5000 ms, and for the first TLS and SSL handshakes, it's 15000 ms, which is too much in so many cases, especially when the esp is connected to a poor network.The ESP goes blind for 15 seconds with every secure request.
You could instead implement a method that checks if the user set a timeout so it respects it and uses it instead of hard-coded ones, If the user didn't set the timeout, you can use your hard-coded timeouts.
Metadata
Metadata
Assignees
Labels
No labels
Activity
d-a-v commentedon Mar 28, 2023
@heshamameen Can you try with #8899 ?
d-a-v commentedon Mar 30, 2023
(updated)
handshake or regular timeout can be set like in the proposed updated example
Arduino/libraries/ESP8266HTTPClient/examples/BasicHttpsClient/BasicHttpsClient.ino
Lines 63 to 64 in cc1951a
heshamameen commentedon Apr 3, 2023
Hi @d-a-v and @mcspr,
Thank you for your response and effort on this issue.
I reviewed your edits and tried them, and overall, they work great and solve the problem of setting the handshake timeout, but I have two comments.
The first one is the new debugging message you added to print time out when starting run_until
DEBUG_BSSL("_run_until starts, timeout=%lu\n", _updateStreamTimeout());It prints too much, especially when you have an open for reuse httpclient so I had to comment it to test with debugging on.
The second is the reset you have implemented
loopTimeout.reset();I don't think it's necessary to reset the timeout inside the run_until loop for two reasons:
While I was trying, I ended up spending 17 seconds on a 5-second timeout request because of the reset.
So I decided to comment reset lines and try again, and it worked almost as I expected. It took almost the timeout in a lot of cases, but other cases took the timeout + a random amount of time spent inside the run_until For example, I set the timeout to 5 seconds, and the loop took 6800 ms with all the resets commented, so with a time-sensitive application, this is disturbing.
d-a-v commentedon Apr 3, 2023
These are fair considerations.
About the debug line, it will be removed.
The Arduino definition of stream timeout is given there. As you can read, it is the maximum time allowed between reading two chars (or the maximum inter-arrival time). This is how ssl-client has been changed in the handshake part, by adding the
.reset()as you noticed.The primary goal of this timeout is to help detect and stop broken connections. Wifi-client-secure has been changed to this end so we don't need to wait for 15s a connection which breaks after 6 seconds.
What you describe is a connection which is too slow for your needs but which will eventually come to a successful end.
I'm interested to know which application needs, on esp8266, a fast SSL connection which is useless when not fast enough, out of curiosity. There are by the way several ways to accelerate ssl connections if you've not tried them yet.
We internally (in fact I internally justified with the above reasons why this PR) came to this proposal:
httpclient::connect()(= ssl handshake) would be out of this overall duration barrier.So this would not solve the handshake issue that you describe @heshamameen.
Maybe @mcspr we can keep the handshake timeout. It would have to be renamed to something else (
setHandshakeDurationBarrier()?) that would kill the connection when user's requirements are not met. This timeout would be infinity by default.mcspr commentedon Apr 4, 2023
I think you also got me confused of your intention there, don't hold my initial agreement against me pls :>
When lower timeouts are used, it does indeed appear to work, but... It is excessive, since socket timeout is already there for individual read() / write() operations. The idea is to catch hanged connections as noted up above - most of the time it would be better to fail early and give control to the app. We deadlock everything otherwise.
Would be really bad.
'Timeout' would mean the same thing as in other contexts - after start of some operation, wait up until this amount of time or abort everything. Duration barrier (thinking of time fence?) sounds pretty weird.
Handshake can also be deferred to first read() / write(), if you worry about the connect() blocking all of a sudden. Then the same logic would apply as the other timeout
heshamameen commentedon Apr 4, 2023
Hi @d-a-v ,
I understand what you say about the internal timeout of the Arduino stream, and of course it makes sense to have a timeout for not receiving anything for a time period to dismiss a broken connection, but I think what you suggested at the end (
setHandshakeDurationBarrier()) is very helpful if implemented because you would give the user the ability to set a time limit for the SSL handshake.This, along with the overall timeout of the httpclient would be a great tool to timeout SSL requests throughout its life time.
As for what kind of application requires this kind of time sensitivity,
For example, I work on a smart wifi switch, and due to cost limitations, I use esp8266, not esp32, so I have to do a lot of operations on a single core, such as listing to an http server, communicating with other microcontrollers that report states that are in sync with esp8266, sending SSL requests to a database to sync states, and a lot more, all at the same time on a single core.
So I have to time everything perfectly to avoid being out of sync, or if the user sends a request to the server and finds the server is not responding, SSL requests are blocking requests, so I have to set a time limit for the request, even if it was sending data, to continue the rest of the communications I have to make through out the loop, and if the request fails, I can schedule it to run another time, and so on.
In such cases, I think @mcspr The duration barrier is not weird at all; it's just meant to be a timing tool to make sure that everything runs under user control and not outside of it.
mcspr commentedon Apr 4, 2023
'wierd' is about the words used, not the timeout / duration / deadline thing itself :)
We can't distinguish WiFiClient from Secure in HTTPClient, hence it makes most sense to make this configurable from WiFiClient side.
Timeout for operation varies between states. When not yet connected, we want total time of operation to be limited b/c it does not make sense to keep not working connection around. After that, polled portion of the timeout is questionable; just keeping the network timeout there can be a bit easier to reason about (without two overlapping timeouts)
d-a-v commentedon Apr 4, 2023
@heshamameen
FWIW, about your background jobs, ticker and recurrent scheduled functions are still called while esp is busy within slow functions like ssl handshake.
d-a-v commentedon Apr 5, 2023
@heshamameen
There is an update within #8899.
Can you try it and particularly the http-client's new
setWallTime(ms)and see if your constraints can be honored with it ?Usage changes can be seen in the https client example
Arduino/libraries/ESP8266HTTPClient/examples/BasicHttpsClient/BasicHttpsClient.ino
Lines 63 to 64 in ac9d961
heshamameen commentedon Apr 8, 2023
First, thanks for your continued support.
I have read and tested all your added changes for the
_wallTimeaddition.It's a great idea to introduce a timer that terminates the request no matter if we receive data or not while maintaining the timeout parameters to help detect bad and broken connections and interrupt them.
When I tried the
_wallTimechange, it didn't work at all. The only parameter that was effective was the timeout, because you check the wall time expiration in the_wait_for_handshakefunction after calling the_run_until, which is blocking and iterating till the handshake succeeded or the timeout occurred, so it wasn't working because the timer expires when we are stuck in the_run_until, and the checker happens after we leave_run_untilwith success or with a timeout.So I don't think of this
_wallTimefor the handshake only; it can control the time of the whole request by simply checking on its expiration in_run_untilafter the timeout checker, so it will be checked during all the communications in read, write, flush, and handshake and dismiss the request as soon as this time hits then return a high priority error to dismiss the request, and of course this wall time should in this case be forever unless the user changes it.Also, I don't think we need this
_wallTimeinside theHTTPClient, as the library is just a wrapper for Client and Client Secure. The old timeout parameter was sufficient, especially after you propagated it to the client and the ancestors, and the_wallTimetime in Client will be sufficient to return an error toHTTPClientto terminate the request.heshamameen commentedon Apr 9, 2023
I have tried to put the walltime checker inside run until it worked great, but I had some issues with the http stream. I had to set the wall time to zero to make the stream work.
I also tried your nightly build version, and I found that the httpClient resue is not working properly. Usually I do the first patch request to Firebase, for example, in 1250 ms and reuse the connection so that from the second patch it will be 250 ms, but that's not happening in your 0.0.2 version. Every time the request takes 1250 ms as if it's not reused, and the weird thing is that the handshake is not happening for the second time, but it takes 1000 ms in this line
[HTTP-Client] connect: already connected, reusing connectionDebugging output:
the first request before reusing:
the second request after reusing output is:
So I went and tried with the latest release 3.1.2 and it worked perfectly
d-a-v commentedon Jul 18, 2023
@mcspr
Only the handshake walltime would be infinity by default. Leaving it to infinity would be equivalent to the current implementation which relies on regular timeout only.
@heshamameen
If this PR is still relevant, I can go on with it for you to test.
heshamameen commentedon Jul 19, 2023
@d-a-v Of course it is still relevant and it would be great if you investigated more and corrected the wall time-out not working issue.
I have a workaround in the _run_until function to make the wall time-out work for me correctly with this peace of code.
if (_wallTime && !_handshake_done && target == BR_SSL_SENDAPP) { DEBUG_BSSL("_run_until handshake too long\n"); return -1; }So if you can please investigate more and make it work professionally and integrate it in the master branch to be rolled out in the next update to enable us to use it without editing the repo by hand.