WebSockets2_Generic icon indicating copy to clipboard operation
WebSockets2_Generic copied to clipboard

Change QNEthernet client close to use close() instead of stop()

Open ssilverman opened this issue 2 years ago • 6 comments

close() does't wait but stop() does.

ssilverman avatar Jan 31 '22 13:01 ssilverman

I'm assuming that closing without waiting is the preferred behaviour. Am I correct, or would you rather keep the "wait until closed" version?

ssilverman avatar Jan 31 '22 13:01 ssilverman

Thanks for your PR. As I haven't read your recent updated library code, I believe you have clear idea which is better. But it would be very helpful if you can explain in more details the benefits in using close() over stop().

Do you also think I have to change, from stop() to close(, for all the other Ethernet-related libraries?

I'll definitely merge the PR soon.

khoih-prog avatar Jan 31 '22 17:01 khoih-prog

I can’t speak about other libraries, but in my library, close() does the same thing as stop(), except that it doesn’t wait for the connection close to be acknowledged. As for what the right thing is, there isn’t really a correct answer, other than the answer to the question: what would you like the contract of your TcpClient::close() function to be?

Do callers expect an immediate return or do they expect that the connection has actually been shut down? If it takes some time, there could be a timeout delay until that timeout expires or the connection close is acknowledged. “Synchronous” versus “Asynchronous”, really.

ssilverman avatar Jan 31 '22 18:01 ssilverman

One more thing: stop() exists in the Arduino API but close() does not.

ssilverman avatar Jan 31 '22 18:01 ssilverman

I just read your code and the difference is here between stop() and close()

else 
{
  elapsedMillis timer;
  while (conn_->connected && timer < connTimeout_) {
    // NOTE: Depends on Ethernet loop being called from yield()
    yield();
  }
}

I'm a little bit reluctant now to make change as I don't know which one is better yet. I think it's better to wait for more tests and other users to share their thoughts and experience before making decision.

khoih-prog avatar Jan 31 '22 18:01 khoih-prog

Agreed. This was just something I noticed that I thought you might like to consider, even if you don’t take the PR.

ssilverman avatar Jan 31 '22 18:01 ssilverman