Arduino icon indicating copy to clipboard operation
Arduino copied to clipboard

documentation: add description for connected() and status()

Open mcspr opened this issue 2 years ago • 4 comments

ref. the original PR and the discussion https://github.com/esp8266/Arduino/pull/4626 https://github.com/esp8266/Arduino/issues/6701

as mentioned in the https://github.com/esp8266/Arduino/issues/8327#issuecomment-933369314

(I have yet to check the readthedocs rendering)

mcspr avatar Oct 04 '21 22:10 mcspr

And as #6701 already mentioned, this is somewhat different for the secure variant: https://github.com/esp8266/Arduino/blob/9d024d17fd18dffa569fa646b5326b5e8f58b0d5/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp#L259-L264 But, it's unclear whether this refers to the WiFiClient interface or the basic-socket WiFiClient class

mcspr avatar Oct 04 '21 22:10 mcspr

To summarize the need for this change: In the case where connected() was returning true when the connection was closed by peer but data were still available for read in receive buffers, sketches found themselves allowed to write something which obviously leaded to an error when they did. Removing the available() test from connected() could stop this behavior. The downside of this change is that available() has to be checked even if connected() is false. ... and if this (half-closed by peer) connection is fully closed by sketch before reading remaining data, peer complains because its data will never be read. The server example in doc allows to reproduce this, the matching posix error on the peer side shows a 'Broken pipe' if I remember well.

So indeed I think this PR should also remove the available() part of TLS's connected() function for coherency with WiFiClient implementation. Good catch.

But, it's unclear whether this refers to the WiFiClient interface or the basic-socket WiFiClient class

The TLS context is supposed to match the WiFiClient class.

d-a-v avatar Oct 04 '21 22:10 d-a-v

The TLS context is supposed to match the WiFiClient class.

...true, but also note that it is used internally. can update, but it may be better suited as a separate patch

Will update with the 'connected' part with the reasoning, seems like a good place to also explain things

mcspr avatar Oct 05 '21 15:10 mcspr

but also note that it is used internally.

Right! In those places, one must check whether testing available() must be added next to connected(), or replace it.

d-a-v avatar Oct 05 '21 15:10 d-a-v