khoih-prog / WebSockets2_Generic

A WebSocket Server and Client library for Arduino, based on RFC6455, for writing modern Websockets applications. Now support ESP8266, ESP32 (including ESP32-S2 Saola, AI-Thinker ESP-12K, WT32_ETH01, etc.), nRF52, SAMD21, SAMD51, SAM DUE, STM32F/L/H/G/WB/MP1, Teensy, RP2040-based, etc. boards, with WiFiNINA, Teensy 4.1 NativeEthernet/QNEthernet, Ethernet W5x00 / ENC28J60 / LAN8742A / LAN8720, ESP8266 / ESP32-AT modules/shields, as well as SINRIC / Alexa / Google Home
GNU General Public License v3.0
81 stars 30 forks source link

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

Closed ssilverman closed 1 year ago

ssilverman commented 2 years ago

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

ssilverman commented 2 years ago

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

khoih-prog commented 2 years ago

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.

ssilverman commented 2 years ago

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 commented 2 years ago

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

khoih-prog commented 2 years ago

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.

ssilverman commented 2 years ago

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