Inconnect we have to delete the socket object, because all other functions test it for null and then use mutex which is null, because configureSocket was not invoked. And there are too few sockets in total.
InreadSocket the condition of the inner loop was always true and the inner loop never exited to wait for event or timeout on event. The inner loop only paused for yield(). With the PR if no data are available the inner loop doesbreak to outer loop where the thread waits for event or timeout.

~~3) I think we have to do mutex inwrite too~~

facchinm requested review fromandreagilardoni andpennam

July 1, 2024 07:54

Copy link

Contributor

andreagilardoni commentedJul 1, 2024

Hi@JAndrassy, I agree with the changes you made for point 1.
For point 2 are you trying to fix a particular issue that is affecting you? Apart from missing mutex locks and unlocks and the nice code reordering, I don't see any huge changes in code that may impact functionality onMbedClient.
For point 3, I expect it to work without the need of mutex, since I think tx and rx buffer are separated, I will take a look at the lower level implementation in order to be sure

Copy link

ContributorAuthor

JAndrassy commentedJul 1, 2024•
edited
Loading

@andreagilardoni so should we add everywhere toif (sock == NULL),&& mutex == NULL?

https://os.mbed.com/docs/mbed-os/v6.16/mbed-os-api-doxy/class_socket.html#abc36eff670a3bee145f1c629a7eb7ee3
has

If connect() fails it is recommended to close the Socket and create a new one before attempting to reconnect.

I didn't look in the source but I think the unconnected socket still counts as used one from socket-max

Copy link

Contributor

andreagilardoni commentedJul 1, 2024

I think the only method missing the check forsock==nullptr is read(), I think it is enough to return -1 there. I think it is just enough to check forsock, since mutex should be != nullptr when the sock is != nullptr after the changes you proposed here.

The changes on lines 123:124 are necessary.
The changes on line 220, 224 I don't think that may affect anything except performances.

I only need to understand why you performed changes onreadSocket(), since to me everything seems to be a rearrangement on if statements (with some additional actions on mutex). Did you experience any kind of issue that I can try to replicate?

Copy link

ContributorAuthor

JAndrassy commentedJul 1, 2024•
edited
Loading

changes on readSocket()

continue change tobrreak if no data are available to read as I write in the description of the PR

JAndrassy force-pushed thembedclient_readsocket_fixes branch 2 times, most recently from00b5add toa5fc13fCompare

July 2, 2024 04:20

Copy link

ContributorAuthor

JAndrassy commentedJul 2, 2024•
edited
Loading

changes:

for unsuccessful connectsocket->close (it wasdelete sock in the first version of the PR)
usingmutex == null as a check for a not configured client

JAndrassy force-pushed thembedclient_readsocket_fixes branch 2 times, most recently from444339a to00bc011Compare

July 2, 2024 05:07

andreagilardoni reviewed

Jul 2, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp OutdatedShow resolvedHide resolved

andreagilardoni reviewed

Jul 2, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp OutdatedShow resolvedHide resolved

andreagilardoni reviewed

Jul 2, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp OutdatedShow resolvedHide resolved

Copy link

Contributor

andreagilardoni commentedJul 2, 2024

@JAndrassy after looking at that everything seems ok, I need to try to run some examples and then I think we can merge this PR. I like how you reorganized the thread function, thanks for your contribution!

JAndrassy force-pushed thembedclient_readsocket_fixes branch from00bc011 toa40c63fCompare

July 2, 2024 10:37

Copy link

ContributorAuthor

JAndrassy commentedJul 2, 2024

There is one more problem with the readSocket thread, but if I patch the solution in, it screams for a rewrite of the whole inner loop.

the problem is that on err < 0 the thread ends. it can be just that the peer closed the connection. on a newconnect withoutstop(), the connection is established, but the readSocket thread is not started. It is not possible to restart a thread, so it should not end until sock is null.

andreagilardoni previously approved these changes

Jul 3, 2024

View reviewed changes

Copy link

Contributor

andreagilardoni commentedJul 3, 2024

I quickly tested this PR and I don't see any issues with this. About the other issue you are describing, whaterr < 0 are you talking about?

Copy link

ContributorAuthor

JAndrassy commentedJul 3, 2024

sorry ret not err

      if (ret < 0 && ret != NSAPI_ERROR_WOULD_BLOCK) {        mutex->unlock();        goto cleanup;      }

Copy link

Contributor

andreagilardoni commentedJul 3, 2024

If the peer closed the connection than, I think, it is expected that the socket has to be closed and needs to be restarted. In connect I can see that this check is performed

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

Lines 84 to 89 in2ece915

	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();
	}

Did I get your point?

Copy link

ContributorAuthor

JAndrassy commentedJul 3, 2024

then it is ok

maidnl requested changes

Jul 24, 2024

View reviewed changes

Copy link

Contributor

maidnl left a comment

Choose a reason for hiding this comment

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

I tried to analyse and test the proposed changes and I have a few comments (I hope you can find them meaningful).

One thing I found really confusing is the use of the_status flag which is set / reset in a lot of different places (and this make difficult to understand the underling logic).
In thereadSocket() thread is appears that is not really necessary to set_status=true at the end of each innermostdo-while cicle.
There is no need to set_status=true after the call of theconfigureSocket() function since is theconfigureSocket() function itself sets_status=true;. This happens twice: in theconnect() and theconnectSSL() function.
As additional simplification I would reset_status to false at the beginning of those 2 functions (if all goes well theconfigureSocket() function will set it totrue and in case of problem it will remain set tofalse removing the need of setting if tofalse in case of errors). This removes the need to set_status to false in case of problem and ensure it is false if something wrong happened.

About the mutex lock/unlock in thewrite() function, my understanding is that the mutex is used to prevent access to therxBuffer from different threads, so it appears to be not necessary here. One point that is certainly wrong is that the changes removed the check about thesock pointer: it is necessary to reintroduce here the check

if (sock == nullptr) {   return 0;}

this is very important because a user can callwrite() with an invalidsock and this would crash the program.

Since the mutex is used to prevent "common" access torxBuffer I noticed that thepeek() function that usesrxBuffer is unprotected: I would add the mutex lock/unlock mechanism to thepeek() function.

A possible improvement is related to thesetSocket() function: this function is called when client is create by a server. However the server allocates aSocket only if there is an incoming request, in case no request is made the server will always callsetSocket(nullptr) (please check theEthernetServer::available() function in the EthernetServer.cpp file and verify if my understanding is correct).
In case like this it is probably pointless to callconfigureSocket() so I would add a check and execute the body of the function only if_sock is different from nullptr.

My last remark is more a doubt: in theconnect() andconnectSSL() function almost at the end of the function in caseret is not 1 it has been added the statementsock->close(), but this only closes theSocket. Would not be better to call directly theMbedClientstop() function? This would reset all the variables held by the client and not only "close the socket".

libraries/SocketWrapper/src/MbedClient.cppShow resolvedHide resolved

Copy link

ContributorAuthor

JAndrassy commentedJul 24, 2024

_status only exists forstatus(). there is no other simple way. It has no internal use in MbedClient. This PR is not about _status.

I avoid doing unnecessary changes in my PR. Where would I stop if I begin to cleanup the code. So I don't even start. So the superfluous_status=true stay there for this PR.

The readSocket() function runs in a separate thread so it can't switch _status to false if it isn't definitive.

I can remove the lock from write. (btw if socket is null, then mutex is null too)

yes. peek() needs the lock. I add it.

My last remark is more a doubt: in the connect() and connectSSL() function almost at the end of the function in case ret is not 1 it has been added the statement sock->close(), but this only closes the Socket. Would not be better to call directly the MbedClient stop() function? This would reset all the variables held by the client and not only "close the socket".

as I understand it, the idea is that the socket can be reused for next try to connect, that is why it is not deleted. calling stop() would delete it. all other fields are not initialized because configureSocket didn't run

JAndrassy force-pushed thembedclient_readsocket_fixes branch froma40c63f todbe0b07Compare

July 24, 2024 17:59

SocketWrapper MbedClient debugging readSocket

3428d75

JAndrassy force-pushed thembedclient_readsocket_fixes branch fromdbe0b07 to3428d75Compare

July 24, 2024 18:04

Copy link

ContributorAuthor

JAndrassy commentedJul 25, 2024

I can remove the lock from write. (btw if socket is null, then mutex is null too)
yes. peek() needs the lock. I add it.

I made these changes ^^^

Copy link

Member

facchinm commentedSep 5, 2024

Maybe this patch could help fixing#937 ?

schnoberts1 reviewed

Sep 9, 2024

View reviewed changes

libraries/SocketWrapper/src/MbedClient.cpp

		do {
		mutex->lock();
		if (sock !=nullptr && rxBuffer.availableForStore() ==0) {
		if (sock ==nullptr) {

Copy link

schnoberts1Sep 9, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Is this subject to the same issues as "Another racy example" here:https://queue.acm.org/detail.cfm?id=2088916? sock is loop invariant but changed in another thread. mutex is the same isn't it?