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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mcspr
Copy link
Collaborator

@mcspr mcspr commented Oct 4, 2021

ref. the original PR and the discussion
#4626
#6701

as mentioned in the #8327 (comment)

(I have yet to check the readthedocs rendering)

@mcspr mcspr requested a review from d-a-v October 4, 2021 22:21
@mcspr
Copy link
Collaborator Author

mcspr commented Oct 4, 2021

And as #6701 already mentioned, this is somewhat different for the secure variant:

uint8_t WiFiClientSecureCtx::connected() {
if (available() || (_clientConnected() && _handshake_done && (br_ssl_engine_current_state(_eng) != BR_SSL_CLOSED))) {
return true;
}
return false;
}

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

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 4, 2021

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.

@mcspr
Copy link
Collaborator Author

mcspr commented Oct 5, 2021

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

@d-a-v
Copy link
Collaborator

d-a-v commented Oct 5, 2021

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.

connected
~~~~~~~~~

Unlike the reference implementation, ``connected()`` means that the client is available for both reads and writes. It will return ``true`` when the client is ``status() == ESTABLISHED`` or ``available() == true``, but will return ``false`` when the ``status() == CLOSED``. Please use ``status()`` for the connection information, ``availableForWrite()`` to check whether it is possible to write, and ``available()`` if you mean to check whether there's unread data.
Copy link
Collaborator

@d-a-v d-a-v Jan 3, 2022

Choose a reason for hiding this comment

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

It will return true when the client is status() == ESTABLISHED or available() == true, but will return false when the status() == CLOSED.

According to current code, it will return false if the connection is closed, regardless available() result,
meaning "writing is not allowed".

available() can still be checked to tell if something has been received but kept unread.

uint8_t WiFiClient::connected()
{
if (!_client || _client->state() == CLOSED)
return 0;
return _client->state() == ESTABLISHED || available();
}

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

2 participants