Fixed double delete in CAPITimer and add documentation for platform networking CAPI integration (#6994)

Michael Wilkerson-Barker · web-flow · commit a9c8f9948eb1 · 2023-09-25T14:02:07.000-04:00
* Fixed double delete in CAPITimer and added individual functions for handling callbacks.
* Added realm_sync_socket_write_complete() fcn and other updates from review
* Don't call handler functions more than once, esp for timers; updated documentation
* Updated some comments
* Updated changelog after release
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,10 +6,10 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
+* Fixed issue with double delete when using the CAPI for timers in platform networking ([#6993](https://github.com/realm/realm-core/issues/6993), since v13.3.0).
 
 ### Breaking changes
-* None.
+* Platform Networking CAPI has been updated to provide separate functions (instead of 1) for executing callback handlers depending on purpose ([PR #6994](https://github.com/realm/realm-core/pull/6994)).
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.
diff --git a/src/realm.h b/src/realm.h
@@ -4061,6 +4061,26 @@ RLM_API bool realm_mongo_collection_find_one_and_delete(realm_mongodb_collection
                                                         realm_userdata_t data, realm_free_userdata_func_t delete_data,
                                                         realm_mongodb_callback_t callback);
 
+/**
+ * Creates a new sync socket instance for the Sync Client that handles the operations for a custom
+ * websocket and event loop implementation.
+ * @param userdata CAPI implementation specific pointer containing custom context data that is provided to
+ *                 each of the provided functions.
+ * @param userdata_free function that will be called when the sync socket is destroyed to delete userdata. This
+ *                      is required if userdata is not null.
+ * @param post_func function that will be called to post a callback handler onto the event loop - use the
+ *                  realm_sync_socket_post_complete() function when the callback handler is scheduled to run.
+ * @param create_timer_func function that will be called to create a new timer resource with the callback
+ *                          handler that will be run when the timer expires or an erorr occurs - use the
+ *                          realm_sync_socket_timer_canceled() function if the timer is canceled or the
+ *                          realm_sync_socket_timer_complete() function if the timer expires or an error occurs.
+ * @param cancel_timer_func function that will be called when the timer has been canceled by the sync client.
+ * @param free_timer_func function that will be called when the timer resource has been destroyed by the sync client.
+ * @param websocket_connect_func function that will be called when the sync client creates a websocket.
+ * @param websocket_write_func function that will be called when the sync client sends data over the websocket.
+ * @param websocket_free_func function that will be called when the sync client closes the websocket conneciton.
+ * @return a realm_sync_socket_t pointer suitable for passing to realm_sync_client_config_set_sync_socket()
+ */
 RLM_API realm_sync_socket_t* realm_sync_socket_new(
     realm_userdata_t userdata, realm_free_userdata_func_t userdata_free, realm_sync_socket_post_func_t post_func,
     realm_sync_socket_create_timer_func_t create_timer_func,
@@ -4069,17 +4089,94 @@ RLM_API realm_sync_socket_t* realm_sync_socket_new(
     realm_sync_socket_websocket_async_write_func_t websocket_write_func,
     realm_sync_socket_websocket_free_func_t websocket_free_func);
 
-RLM_API void realm_sync_socket_callback_complete(realm_sync_socket_callback_t* realm_callback, realm_errno_e status,
-                                                 const char* reason);
+/**
+ * To be called to execute the callback handler provided to the create_timer_func when the timer is
+ * complete or an error occurs while processing the timer.
+ * @param timer_handler the timer callback handler that was provided when the timer was created.
+ * @param status the error code for the error that occurred or RLM_ERR_NONE if the timer expired normally.
+ * @param reason a string describing details about the error that occurred or empty string if no error.
+ * NOTE: This function must be called by the event loop execution thread.
+ */
+RLM_API void realm_sync_socket_timer_complete(realm_sync_socket_callback_t* timer_handler, realm_errno_e status,
+                                              const char* reason);
 
+/**
+ * To be called to execute the callback handler provided to the create_timer_func when the timer has been
+ * canceled.
+ * @param timer_handler the timer callback handler that was provided when the timer was created.
+ * NOTE: This function must be called by the event loop execution thread.
+ */
+RLM_API void realm_sync_socket_timer_canceled(realm_sync_socket_callback_t* timer_handler);
+
+/**
+ * To be called to execute the callback function provided to the post_func when the event loop executes
+ * that post'ed operation. The post_handler resource will automatically be destroyed during this
+ * operation.
+ * @param post_handler the post callback handler that was originally provided to the post_func
+ * @param status the error code for the error that occurred or RLM_ERR_NONE if the operation was to be run
+ * @param reason a string describing details about the error that occurred or empty string if no error.
+ * NOTE: This function must be called by the event loop execution thread.
+ */
+RLM_API void realm_sync_socket_post_complete(realm_sync_socket_callback_t* post_handler, realm_errno_e status,
+                                             const char* reason);
+
+/**
+ * To be called to execute the callback function provided to the websocket_write_func when the write
+ * operation is complete. The write_handler resource will automatically be destroyed during this
+ * operation.
+ * @param write_handler the write callback handler that was originally provided to the websocket_write_func
+ * @param status the error code for the error that occurred or RLM_ERR_NONE if write completed successfully
+ * @param reason a string describing details about the error that occurred or empty string if no error.
+ * NOTE: This function must be called by the event loop execution thread.
+ */
+RLM_API void realm_sync_socket_write_complete(realm_sync_socket_callback_t* write_handler, realm_errno_e status,
+                                              const char* reason);
+
+/**
+ * To be called when the websocket successfully connects to the server.
+ * @param realm_websocket_observer the websocket observer object that was provided to the websocket_connect_func
+ * @param protocol the value of the Sec-WebSocket-Protocol header in the connect response from the server.
+ * NOTE: This function must be called by the event loop execution thread and should not be called
+ *       after the websocket_free_func has been called to release the websocket resources.
+ */
 RLM_API void realm_sync_socket_websocket_connected(realm_websocket_observer_t* realm_websocket_observer,
                                                    const char* protocol);
 
+/**
+ * To be called when an error occurs - the actual error value will be provided when the websocket_closed
+ * function is called. This function informs that the socket object is in an error state and no further
+ * TX operations should be performed.
+ * @param realm_websocket_observer the websocket observer object that was provided to the websocket_connect_func
+ * NOTE: This function must be called by the event loop execution thread and should not be called
+ *       after the websocket_free_func has been called to release the websocket resources.
+ */
 RLM_API void realm_sync_socket_websocket_error(realm_websocket_observer_t* realm_websocket_observer);
 
+/**
+ * To be called to provide the received data to the Sync Client when a write operation has completed.
+ * The data buffer can be safely discarded after this function has completed.
+ * @param realm_websocket_observer the websocket observer object that was provided to the websocket_connect_func
+ * @param data a pointer to the buffer that contains the data received over the websocket
+ * @param data_size the number of bytes in the data buffer
+ * NOTE: This function must be called by the event loop execution thread and should not be called
+ *       after the websocket_free_func has been called to release the websocket resources.
+ */
 RLM_API void realm_sync_socket_websocket_message(realm_websocket_observer_t* realm_websocket_observer,
                                                  const char* data, size_t data_size);
 
+/**
+ * To be called when the websocket has been closed, either due to an error or a normal close operation.
+ * @param realm_websocket_observer the websocket observer object that was provided to the websocket_connect_func
+ * @param was_clean boolean value that indicates whether this is a normal close situation (true), the
+ *                  error was provided by the server via a close message (true), or if the error was
+ *                  generated by the local websocket as a result of some other error (false) (e.g. host
+ *                  unreachable, etc.)
+ * @param status the websocket error code that describes why the websocket was closed, or
+ *               RLM_ERR_WEBSOCKET_OK if the socket was closed normally.
+ * @param reason a string describing details about the error that occurred or empty string if no error.
+ * NOTE: This function must be called by the event loop execution thread and should not be called
+ *       after the websocket_free_func has been called to release the websocket resources.
+ */
 RLM_API void realm_sync_socket_websocket_closed(realm_websocket_observer_t* realm_websocket_observer, bool was_clean,
                                                 realm_web_socket_errno_e status, const char* reason);
 
diff --git a/src/realm/object-store/c_api/socket_provider.cpp b/src/realm/object-store/c_api/socket_provider.cpp
@@ -7,13 +7,16 @@
 namespace realm::c_api {
 namespace {
 
+// THis class represents the timer resource that is returned to the sync client from the
+// CAPI implementation details for canceling and deleting the timer resources.
 struct CAPITimer : sync::SyncSocketProvider::Timer {
 public:
     CAPITimer(realm_userdata_t userdata, int64_t delay_ms, realm_sync_socket_callback_t* handler,
               realm_sync_socket_create_timer_func_t create_timer_func,
               realm_sync_socket_timer_canceled_func_t cancel_timer_func,
               realm_sync_socket_timer_free_func_t free_timer_func)
         : m_handler(handler)
+        , m_userdata(userdata)
         , m_timer_create(create_timer_func)
         , m_timer_cancel(cancel_timer_func)
         , m_timer_free(free_timer_func)
@@ -24,27 +27,65 @@ struct CAPITimer : sync::SyncSocketProvider::Timer {
     /// Cancels the timer and destroys the timer instance.
     ~CAPITimer()
     {
+        // Make sure the timer is stopped, if not already
         m_timer_cancel(m_userdata, m_timer);
         m_timer_free(m_userdata, m_timer);
         realm_release(m_handler);
     }
 
-    /// Cancel the timer immediately.
+    // Cancel the timer immediately - the CAPI implementation will need to call the
+    // realm_sync_socket_timer_canceled function to notify the sync client that the
+    // timer has been canceled and must be called in the same execution thread as
+    // the timer complete.
     void cancel() override
     {
         m_timer_cancel(m_userdata, m_timer);
     }
 
 private:
+    // A pointer to the CAPI implementation's timer instance. This is provided by the
+    // CAPI implementation when the create_timer_func function is called.
     realm_sync_socket_timer_t m_timer = nullptr;
 
-    realm_userdata_t m_userdata = nullptr;
+    // A wrapped reference to the callback function to be called when the timer completes,
+    // is canceled or an error occurs. This is provided by the Sync Client
     realm_sync_socket_callback_t* m_handler = nullptr;
+
+    // These values were originally provided to the socket_provider instance by the CAPI
+    // implementation when it was created
+    realm_userdata_t m_userdata = nullptr;
     realm_sync_socket_create_timer_func_t m_timer_create = nullptr;
     realm_sync_socket_timer_canceled_func_t m_timer_cancel = nullptr;
     realm_sync_socket_timer_free_func_t m_timer_free = nullptr;
 };
 
+static void realm_sync_socket_op_complete(realm_sync_socket_callback* realm_callback, realm_errno_e status,
+                                          const char* reason)
+{
+    if (realm_callback->get() != nullptr) {
+        auto complete_status = status == realm_errno_e::RLM_ERR_NONE
+                                   ? Status::OK()
+                                   : Status{static_cast<ErrorCodes::Error>(status), reason};
+        (*(realm_callback->get()))(complete_status);
+        // Keep the container, but release the handler so it can't be called twice.
+        realm_callback->reset();
+    }
+}
+
+RLM_API void realm_sync_socket_timer_complete(realm_sync_socket_callback* timer_handler, realm_errno_e status,
+                                              const char* reason)
+{
+    realm_sync_socket_op_complete(timer_handler, status, reason);
+}
+
+RLM_API void realm_sync_socket_timer_canceled(realm_sync_socket_callback* timer_handler)
+{
+    realm_sync_socket_op_complete(timer_handler, RLM_ERR_OPERATION_ABORTED, "Timer canceled");
+}
+
+// This class represents a websocket instance provided by the CAPI implememtation for sending
+// and receiving data and connection state from the websocket. This class is used directly by
+// the sync client.
 struct CAPIWebSocket : sync::WebSocketInterface {
 public:
     CAPIWebSocket(realm_userdata_t userdata, realm_sync_socket_connect_func_t websocket_connect_func,
@@ -89,15 +130,24 @@ struct CAPIWebSocket : sync::WebSocketInterface {
     }
 
 private:
+    // A pointer to the CAPI implementation's websocket instance. This is provided by
+    // the m_websocket_connect() function when this websocket instance is created.
     realm_sync_socket_websocket_t m_socket = nullptr;
+
+    // A wrapped reference to the websocket observer in the sync client that receives the
+    // websocket status callbacks. This is provided by the Sync Client.
     realm_websocket_observer_t* m_observer = nullptr;
-    realm_userdata_t m_userdata = nullptr;
 
+    // These values were originally provided to the socket_provider instance by the CAPI
+    // implementation when it was created.
+    realm_userdata_t m_userdata = nullptr;
     realm_sync_socket_connect_func_t m_websocket_connect = nullptr;
     realm_sync_socket_websocket_async_write_func_t m_websocket_async_write = nullptr;
     realm_sync_socket_websocket_free_func_t m_websocket_free = nullptr;
 };
 
+// Represents the websocket observer in the sync client that receives websocket status
+// callbacks and passes them along to the WebSocketObserver object.
 struct CAPIWebSocketObserver : sync::WebSocketObserver {
 public:
     CAPIWebSocketObserver(std::unique_ptr<sync::WebSocketObserver> observer)
@@ -131,9 +181,13 @@ struct CAPIWebSocketObserver : sync::WebSocketObserver {
     std::unique_ptr<sync::WebSocketObserver> m_observer;
 };
 
+// This is the primary resource for providing event loop, timer and websocket
+// resources and synchronization for the Sync Client. The CAPI implementation
+// needs to implement the "funct_t" functions provided to this class for connecting
+// the implementation to the operations called by the Sync Client.
 struct CAPISyncSocketProvider : sync::SyncSocketProvider {
     realm_userdata_t m_userdata = nullptr;
-    realm_free_userdata_func_t m_free = nullptr;
+    realm_free_userdata_func_t m_userdata_free = nullptr;
     realm_sync_socket_post_func_t m_post = nullptr;
     realm_sync_socket_create_timer_func_t m_timer_create = nullptr;
     realm_sync_socket_timer_canceled_func_t m_timer_cancel = nullptr;
@@ -145,7 +199,7 @@ struct CAPISyncSocketProvider : sync::SyncSocketProvider {
     CAPISyncSocketProvider() = default;
     CAPISyncSocketProvider(CAPISyncSocketProvider&& other)
         : m_userdata(std::exchange(other.m_userdata, nullptr))
-        , m_free(std::exchange(other.m_free, nullptr))
+        , m_userdata_free(std::exchange(other.m_userdata_free, nullptr))
         , m_post(std::exchange(other.m_post, nullptr))
         , m_timer_create(std::exchange(other.m_timer_create, nullptr))
         , m_timer_cancel(std::exchange(other.m_timer_cancel, nullptr))
@@ -154,7 +208,10 @@ struct CAPISyncSocketProvider : sync::SyncSocketProvider {
         , m_websocket_async_write(std::exchange(other.m_websocket_async_write, nullptr))
         , m_websocket_free(std::exchange(other.m_websocket_free, nullptr))
     {
-        REALM_ASSERT(m_free);
+        // userdata_free can be null if userdata is not used
+        if (m_userdata != nullptr) {
+            REALM_ASSERT(m_userdata_free);
+        }
         REALM_ASSERT(m_post);
         REALM_ASSERT(m_timer_create);
         REALM_ASSERT(m_timer_cancel);
@@ -166,9 +223,14 @@ struct CAPISyncSocketProvider : sync::SyncSocketProvider {
 
     ~CAPISyncSocketProvider()
     {
-        m_free(m_userdata);
+        if (m_userdata_free) {
+            m_userdata_free(m_userdata);
+        }
     }
 
+    // Create a websocket object that will be returned to the Sync Client, which is expected to
+    // begin connecting to the endpoint as soon as the object is created. The state and any data
+    // received is passed to the socket observer via the helper functions defined below this class.
     std::unique_ptr<sync::WebSocketInterface> connect(std::unique_ptr<sync::WebSocketObserver> observer,
                                                       sync::WebSocketEndpoint&& endpoint) final
     {
@@ -206,7 +268,7 @@ RLM_API realm_sync_socket_t* realm_sync_socket_new(
     return wrap_err([&]() {
         auto capi_socket_provider = std::make_shared<CAPISyncSocketProvider>();
         capi_socket_provider->m_userdata = userdata;
-        capi_socket_provider->m_free = userdata_free;
+        capi_socket_provider->m_userdata_free = userdata_free;
         capi_socket_provider->m_post = post_func;
         capi_socket_provider->m_timer_create = create_timer_func;
         capi_socket_provider->m_timer_cancel = cancel_timer_func;
@@ -218,13 +280,18 @@ RLM_API realm_sync_socket_t* realm_sync_socket_new(
     });
 }
 
-RLM_API void realm_sync_socket_callback_complete(realm_sync_socket_callback* realm_callback, realm_errno_e code,
-                                                 const char* reason)
+RLM_API void realm_sync_socket_post_complete(realm_sync_socket_callback* post_handler, realm_errno_e status,
+                                             const char* reason)
+{
+    realm_sync_socket_op_complete(post_handler, status, reason);
+    realm_release(post_handler);
+}
+
+RLM_API void realm_sync_socket_write_complete(realm_sync_socket_callback_t* write_handler, realm_errno_e status,
+                                              const char* reason)
 {
-    auto complete_status =
-        code == realm_errno_e::RLM_ERR_NONE ? Status::OK() : Status{static_cast<ErrorCodes::Error>(code), reason};
-    (*(realm_callback->get()))(complete_status);
-    realm_release(realm_callback);
+    realm_sync_socket_op_complete(write_handler, status, reason);
+    realm_release(write_handler);
 }
 
 RLM_API void realm_sync_socket_websocket_connected(realm_websocket_observer_t* realm_websocket_observer,
@@ -245,10 +312,10 @@ RLM_API void realm_sync_socket_websocket_message(realm_websocket_observer_t* rea
 }
 
 RLM_API void realm_sync_socket_websocket_closed(realm_websocket_observer_t* realm_websocket_observer, bool was_clean,
-                                                realm_web_socket_errno_e code, const char* reason)
+                                                realm_web_socket_errno_e status, const char* reason)
 {
     realm_websocket_observer->get()->websocket_closed_handler(
-        was_clean, static_cast<sync::websocket::WebSocketError>(code), reason);
+        was_clean, static_cast<sync::websocket::WebSocketError>(status), reason);
 }
 
 RLM_API void realm_sync_client_config_set_sync_socket(realm_sync_client_config_t* config,
diff --git a/test/object-store/c_api/c_api.cpp b/test/object-store/c_api/c_api.cpp
