Implemented close correctly (#4002)

* Implemented close correctly

* Code review feedback - made several classes inal.

* clang-format

sdk/core/azure-core/inc/azure/core/http/websockets/websockets.hpp

@@ -62,7 +62,7 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
      * Note: Some of these statistics are not available if the underlying transport supports native
      * websockets.
      */
-    struct WebSocketStatistics
+    struct WebSocketStatistics final
     {
       /** @brief The number of WebSocket frames sent on this WebSocket. */
       uint32_t FramesSent;
@@ -172,11 +172,12 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
           : FrameType{frameType}, IsFinalFrame{isFinalFrame}
       {
       }
+      virtual ~WebSocketFrame() {}
     };
 
     /** @brief Contains the contents of a WebSocket Text frame.*/
-    class WebSocketTextFrame : public WebSocketFrame,
-                               public std::enable_shared_from_this<WebSocketTextFrame> {
+    class WebSocketTextFrame final : public WebSocketFrame,
+                                     public std::enable_shared_from_this<WebSocketTextFrame> {
       friend Azure::Core::Http::WebSockets::_detail::WebSocketImplementation;
 
     private:
@@ -201,8 +202,8 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
     };
 
     /** @brief Contains the contents of a WebSocket Binary frame.*/
-    class WebSocketBinaryFrame : public WebSocketFrame,
-                                 public std::enable_shared_from_this<WebSocketBinaryFrame> {
+    class WebSocketBinaryFrame final : public WebSocketFrame,
+                                       public std::enable_shared_from_this<WebSocketBinaryFrame> {
       friend Azure::Core::Http::WebSockets::_detail::WebSocketImplementation;
 
     private:
@@ -227,8 +228,9 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
     };
 
     /** @brief Contains the contents of a WebSocket Close frame.*/
-    class WebSocketPeerCloseFrame : public WebSocketFrame,
-                                    public std::enable_shared_from_this<WebSocketPeerCloseFrame> {
+    class WebSocketPeerCloseFrame final
+        : public WebSocketFrame,
+          public std::enable_shared_from_this<WebSocketPeerCloseFrame> {
       friend Azure::Core::Http::WebSockets::_detail::WebSocketImplementation;
 
     public:
@@ -254,7 +256,7 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
       }
     };
 
-    struct WebSocketOptions : Azure::Core::_internal::ClientOptions
+    struct WebSocketOptions final : Azure::Core::_internal::ClientOptions
     {
       /**
        * @brief The set of protocols which are supported by this client
@@ -290,7 +292,7 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
       WebSocketOptions() = default;
     };
 
-    class WebSocket {
+    class WebSocket final {
     public:
       /** @brief Constructs a new instance of a WebSocket with the specified WebSocket options.
        *
@@ -311,13 +313,12 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets {
        */
       void Open(Azure::Core::Context const& context = Azure::Core::Context{});
 
-      /** @brief Closes a WebSocket connection to the remote server gracefully.
-       *
-       * @param context Context for the operation.
+      /** @brief Closes a WebSocket connection to the remote server.
        */
-      void Close(Azure::Core::Context const& context = Azure::Core::Context{});
+      void Close();
 
-      /** @brief Closes a WebSocket connection to the remote server with additional context.
+      /** @brief Gracefully closes a WebSocket connection to the remote server with additional
+       * context.
        *
        * @param closeStatus 16 bit WebSocket error code.
        * @param closeReason String describing the reason for closing the socket.

sdk/core/azure-core/src/http/curl/curl_websockets.cpp

@@ -27,7 +27,13 @@
 
 namespace Azure { namespace Core { namespace Http { namespace WebSockets {
 
-  void CurlWebSocketTransport::Close() { m_upgradedConnection->Shutdown(); }
+  void CurlWebSocketTransport::Close()
+  {
+    if (m_upgradedConnection)
+    {
+      m_upgradedConnection->Shutdown();
+    }
+  }
 
   // Send an HTTP request to the remote server.
   std::unique_ptr<RawResponse> CurlWebSocketTransport::Send(

sdk/core/azure-core/src/http/websockets/websockets.cpp

@@ -21,11 +21,7 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
   {
     m_socketImplementation->Open(context);
   }
-  void WebSocket::Close(Azure::Core::Context const& context)
-  {
-    m_socketImplementation->Close(
-        static_cast<uint16_t>(WebSocketErrorCode::EndpointDisappearing), {}, context);
-  }
+  void WebSocket::Close() { m_socketImplementation->Close(); }
   void WebSocket::Close(
       uint16_t closeStatus,
       std::string const& closeReason,

sdk/core/azure-core/src/http/websockets/websockets_impl.cpp

@@ -48,6 +48,8 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
   {
   }
 
+  WebSocketImplementation::~WebSocketImplementation() { Close(); }
+
   void WebSocketImplementation::Open(Azure::Core::Context const& context)
   {
     if (m_state != SocketState::Invalid && m_state != SocketState::Closed)
@@ -197,6 +199,19 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
     m_headers.emplace(std::make_pair(header, headerValue));
   }
 
+  void WebSocketImplementation::Close()
+  {
+    std::unique_lock<std::mutex> lock(m_stateMutex);
+    m_stateOwner = std::this_thread::get_id();
+    // Close the socket - after this point, the m_transport is invalid.
+    m_pingThread.Shutdown();
+    if (m_transport)
+    {
+      m_transport->Close();
+    }
+    m_state = SocketState::Closed;
+  }
+
   void WebSocketImplementation::Close(
       uint16_t closeStatus,
       std::string const& closeReason,
@@ -253,10 +268,8 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
       lock.lock();
       m_stateOwner = std::this_thread::get_id();
     }
-    // Close the socket - after this point, the m_transport is invalid.
-    m_pingThread.Shutdown();
-    m_transport->Close();
-    m_state = SocketState::Closed;
+    lock.unlock();
+    Close();
   }
 
   void WebSocketImplementation::SendFrame(

sdk/core/azure-core/src/http/websockets/websockets_impl.hpp

@@ -16,7 +16,7 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
   // Generator for random bytes. Used in WebSocketImplementation and tests.
   std::vector<uint8_t> GenerateRandomBytes(size_t vectorSize);
 
-  class WebSocketImplementation {
+  class WebSocketImplementation final {
     enum class SocketState
     {
       Invalid,
@@ -30,8 +30,10 @@ namespace Azure { namespace Core { namespace Http { namespace WebSockets { names
     WebSocketImplementation(
         Azure::Core::Url const& remoteUrl,
         _internal::WebSocketOptions const& options);
+    ~WebSocketImplementation();
 
     void Open(Azure::Core::Context const& context);
+    void Close();
     void Close(
         uint16_t closeStatus,
         std::string const& closeReason,