Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Sep 25, 2023
Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
* If a user was logged out while an access token refresh was in progress, the refresh completing would mark the user as logged in again and the user would be in an inconsistent state ([PR #6837](https://github.com/realm/realm-core/pull/6837), since v10.0.0).
* If querying over a geospatial dataset that had some objects with a type property set to something other than 'Point' (case insensitive) an exception would have been thrown. Instead of disrupting the query, those objects are now just ignored. ([PR 6989](https://github.com/realm/realm-core/issues/6989), since the introduction of geospatial)
* The Swift package failed to link required libraries when building for macCatalyst.
* 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
* SyncUser::provider_type() and realm_user_get_auth_provider() have been removed. Users don't have provider types; identities do. `SyncUser::is_anonymous()` is a more correct version of checking if the provider type is anonymous ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
* SyncUser no longer has a `local_identity()`. `identity()` has been guaranteed to be unique per App ever since v10 ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
* SyncUser no longer overrides operator==. Pointer equality should be used to compare sync users ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
* 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.
Expand Down
100 changes: 98 additions & 2 deletions src/realm.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -4069,17 +4089,93 @@ 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

This used to be called after a websocket_async_write_func to indicate error/complete via the callback provided in

realm_sync_socket_callback_t* realm_callback);

we can use alternatively realm_sync_socket_post_complete ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right - I forgot about that callback. I'll create a new function realm_sync_socket_write_complete() so it's obvious when it should be called (even though it does the same as post_complete)

const char* reason);
/**
* This function is to be called by the CAPI Implementer when the timer expires or an error occurs.
* @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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

aren't these two new api calls leaking the callback if realm_sync_socket_post_complete is never called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two callbacks are meant to be used with the CAPITimer, which will release the callback handler when the timer is destroyed.
Do you think it would be better to not call realm_release(m_handler) in the destructor and rely on the call to timer_complete/timer_canceled to delete m_handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should rethink the callback handler a bit differently so the callback isn't called multiple times...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's an option. I just saw you kept the release in dtor, so there is no leak. But, how is double free gone? (can't the callback be invoked followed by release, and then when the timer is gone there is a new release?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The double free is gone, since you're supposed to use the timer_complete() or timer_canceled() fcns, which doesn't call release, so there's only one release when the timer is destroyed (if the CAPI is implemented properly), however, the callback could be called multiple times depending on how the timer is implemented. (cancel is always called in the destructor to make sure the timer is stopped)

Copy link
Collaborator

@danieltabacaru danieltabacaru Sep 22, 2023

Choose a reason for hiding this comment

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

but post calls release.. That was the initial issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But you're not supposed to use post (or write_complete) with the timer handler, which are different objects from the callback handlers provided to the post or async_write functions.

const char* reason);

/**
* This function is to be called by the CAPI implementer when the timer is canceled by the Sync Client, either
* directly or as a result of the CAPITimer object being destroyed.
* @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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should test the new api calls

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could even recreate the initial issue using TestSyncTimer in c_api.cpp


/**
* 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);

Expand Down
Loading