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.

Sign up for GitHub

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

enforcing stop() when calling connect a second time #949

Merged
merged 1 commit into from
Oct 2, 2024
Merged

enforcing stop() when calling connect a second time #949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions libraries/SocketWrapper/src/MbedClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,8 @@ void arduino::MbedClient::configureSocket(Socket *_s) {
}

int arduino::MbedClient::connect(SocketAddress socketAddress) {
Copy link

Choose a reason for hiding this comment

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

In connect and connectSSL I see a discrepancy in how timesout are set.
Can you please take a look and make them uniform?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as stated here TLSSocketWrapper requires the timeout to be set before the connect is called. In the plain TCP version we call set_timeout before write, I think the reason for this is to update the timeout value internally if the user changes it. I would not touch it for the time being. @pennam can you confirm that?

ArduinoCore-mbed/libraries/SocketWrapper/src/MbedClient.cpp

Lines 154 to 157 in 702daa0

/* For TLS connection timeout needs to be configured before handshake starts
* otherwise socket timeout is not adopted. See TLSSocketWrapper::set_timeout(int timeout)
*/
sock->set_timeout(_timeout);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we can add a sock-> set_timeout() also in arduino::MbedClient::connect(SocketAddress socketAddress) just before nsapi_error_t returnCode = static_cast<TCPSocket *>(sock)->connect(socketAddress); since also TCPSocket::connect(const SocketAddress &address) is using the _timeout value, but i would do it in a separate PR.


if (sock && reader_th) {
// trying to reuse a connection, let's call stop() to cleanup the state
char c;
if (sock->recv(&c, 1) < 0) {
stop();
}
}
// if a connection is aready ongoing, a disconnection must be enforced before starting another one
stop();
Copy link

Choose a reason for hiding this comment

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

ok. I don't know the reason why the the socker->recv was there... Do you know why?
Maybe better to re-initialize always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why it was there, but I suppose that if someone calls connect he wants to reset the connection before connecting, since I could connect to a new host


if (sock == nullptr) {
sock = new TCPSocket();
Expand Down Expand Up @@ -135,6 +129,9 @@ int arduino::MbedClient::connect(const char *host, uint16_t port) {
}

int arduino::MbedClient::connectSSL(SocketAddress socketAddress) {
// if a connection is aready ongoing, a disconnection must be enforced before starting another one
stop();

if (sock == nullptr) {
sock = new TLSSocket();
_own_socket = true;
Expand Down
Loading