Uniform behaviour of WiFiClientSecure and WiFiClient setTimeout()

[gonzabrusco]

Summary

WiFiClientSecure setTimeout does nothing (literally nothing), while WiFiClient setTimeout really modifies the socket send and receive timeouts. This is not an uniform behaviour and I think this should be fixed. This is my proposal but I would like you to review it very thoroughly in case I'm ommiting something.

Now both classes have a class variable called _timeout. These variables are setted to the original timeout defaults in the constructors of each class (3000 ms for WiFiClient and 30000ms for WiFiClientSecure).

setTimeout() now sets this variable but also tries to configure the socket in case the connection was previously established (this is the original WiFiClient behaviour except for the class variable setting). This variable (_timeout) will be used for each new connection WiFiClient and WiFiClientSecure make.

In case of WiFiClientSecure there's also setHandshakeTimeout(), this is kept as is, but any interaction between _timeout and sslclient->handshake_timeout was removed because now they should remain independent timeouts.

Previous to this PR, you could not set the send and receive timeouts of WiFiClientSecure. They were always forced to 30000 ms. Now the behaviour is similar to WiFiClient.
Also previously with WiFiClient, if you set the timeout BEFORE the connection, it would end up ignored because the socket was not created and the configuration was not saved (there was no _timeout class variable).

Honestly I must admit that I felt a bit surprised to see these inconsistencies on such an important part of the arduino core. Please review it and let me know what you think. This is just a proposal but feel free to change everything you want.

Impact

Now you can call setTimeout() before a connection in WiFiClient.
Now you can call setTimeout() with a WiFiClientSecure and actually set something...
WiFiClientSecure send and receive timeouts now are independent of the handshake timeout.

Related links

I created this bug report a few days ago (#6429 (comment))

[github-actions]

Unit Test Results

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌

Results for commit 634cca3.

♻️ This comment has been updated with latest results.

[dexion]

Why did you not set Stream's timeout in WifiClientSecure like Client::setTimeout(seconds * 1000); ?
_timeout variable is overloaded in ancestor and it wouldn't be set for Stream's functions like Stream::readStringUntil which uses Stream::timedRead? BTW _timeout should not be int, it can't be less zero. I would prefer use unsigned int for clarity )

[gonzabrusco]

Why did you not set Stream's timeout in WifiClientSecure like Client::setTimeout(seconds * 1000); ? _timeout variable is overloaded in ancestor and it wouldn't be set for Stream's functions like Stream::readStringUntil which uses Stream::timedRead? BTW _timeout should not be int, it can't be less zero. I would prefer use unsigned int for clarity )

About the type, I used int following what WiFiClientSecure used originally, to be consistent. Also, all the prototypes of the class use "int32_t timeout" as the passing type for the timeout. So I respected that too. Nevertheless, I agree with you that maybe all should be an unsigned type.

About the Client::setTimeout(seconds * 1000);, actually I didn't do It because I wasn't sure if WiFiClient should do it in the first place. What they are modifying calling the Client::Timeout is the timeout in the Stream Class as you said. But I think that with WiFiClient and WiFiClientSecure that timeout is meaningless because everything is handled at the LWIP level. But I could be wrong, I wasn't really sure about that so I decided to "not change it".

[me-no-dev]

Hi @gonzabrusco ! Your implementation looks great. Thanks!