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

michael-wb · 2023-09-21T18:56:37Z

What, How & Why?

There was a double delete occurring of the callback handler when a CAPITimer object was destroyed in the CAPI implementation after having called realm_sync_socket_callback_complete() to report timer status. Broke up the realm_sync_socket_callback_complete() to use 4 separate functions instead:

realm_sync_socket_timer_complete() - to be used when a timer expires or an error occurs
realm_sync_socket_timer_canceled() - to be used when a timer has been canceled
realm_sync_socket_post_complete() - to be used when a callback handler added using post() is ready to run
realm_sync_socket_write_complete() - to be used when a websocket write operation has completed

The CAPITimer's callback handler will only be destroyed when the CAPITimer object is destroyed.

Added documentation to the Platform Networking functions in realm.h to describe their purpose to help with future custom platform networking implementations.

FIxes #6993

☑️ ToDos

📝 Changelog update
~~[ ] 🚦 Tests (or not relevant)~~
~~[ ] C-API, if public C++ API changed.~~

…andling callbacks.

coveralls-official · 2023-09-21T19:38:08Z

Pull Request Test Coverage Report for Build michael.wilkersonbarker_855

20 of 33 (60.61%) changed or added relevant lines in 2 files are covered.
75 unchanged lines in 13 files lost coverage.
Overall coverage increased (+0.04%) to 91.242%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/realm/object-store/c_api/socket_provider.cpp	19	32	59.38%

Files with Coverage Reduction	New Missed Lines	%
src/realm/object-store/c_api/socket_provider.cpp	2	66.43%
src/realm/object-store/sync/sync_manager.cpp	2	86.96%
src/realm/sync/instruction_applier.cpp	2	70.21%
src/realm/sync/noinst/server/server_history.cpp	2	67.69%
test/test_lang_bind_helper.cpp	2	93.3%
src/realm/sync/noinst/client_reset_operation.cpp	4	87.23%
src/realm/sync/transform.cpp	4	61.71%
src/realm/unicode.cpp	4	90.15%
src/realm/index_string.cpp	6	87.76%
src/realm/bplustree.cpp	7	75.72%

Totals
Change from base Build 1704:	0.04%
Covered Lines:	232543
Relevant Lines:	254864

💛 - Coveralls

nhachicha

I tested this branch & it fixed the double delete issue 👍 left some minor comment

nhachicha · 2023-09-22T09:47:57Z

src/realm.h

 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 with be provided when the websocket_closed


Suggested change

* To be called when an error occurs - the actual error value with be provided when the websocket_closed

* To be called when an error occurs - the actual error value will be provided when the websocket_closed

nhachicha · 2023-09-22T10:59:35Z

src/realm.h

@@ -4069,17 +4088,79 @@ 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,


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

realm-core/src/realm.h

Line 383 in f56d770

realm_sync_socket_callback_t* realm_callback);

we can use alternatively realm_sync_socket_post_complete ...

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)

nhachicha · 2023-09-22T11:20:49Z

src/realm/object-store/c_api/socket_provider.cpp

+
+    // These values were originally provided to the socket_provider instance by the CAPI
+    // implementation when it was created
+    realm_userdata_t m_userdata = nullptr;


I don't think this is initialized via the ctor, it is also passed to

realm-core/src/realm/object-store/c_api/socket_provider.cpp

Lines 29 to 30 in f56d770

m_timer_cancel(m_userdata, m_timer);

m_timer_free(m_userdata, m_timer);

This is an overlook, so it should be.

…eview

danieltabacaru · 2023-09-22T19:49:55Z

src/realm.h

+ * @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,


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

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?

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

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?)

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)

but post calls release.. That was the initial issue

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.

danieltabacaru · 2023-09-22T21:28:09Z

src/realm.h

+ * @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);


we should test the new api calls

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

…documentation

danieltabacaru

Looks good. I would add a few more tests though.

michael-wb · 2023-09-22T21:56:19Z

Looks good. I would add a few more tests though.

Created RCORE-1793/ #6998 to add more tests as a separate task

nhachicha

Tested again using the new realm_sync_socket_write_complete looks good!

…itimer-double-delete

michael-wb self-assigned this Sep 21, 2023

cla-bot bot added the cla: yes label Sep 21, 2023

Michael Wilkerson-Barker added 2 commits September 21, 2023 15:05

Fixed double delete in CAPITimer and added individual functions for h…

1d9e631

…andling callbacks.

Updated changelog

f56d770

michael-wb force-pushed the mwb/fix-capitimer-double-delete branch from d672cd4 to f56d770 Compare September 21, 2023 19:46

michael-wb requested review from danieltabacaru and nhachicha September 21, 2023 19:48

nhachicha reviewed Sep 22, 2023

View reviewed changes

Added realm_sync_socket_write_complete() fcn and other updates from r…

32cb70d

…eview

michael-wb requested a review from nhachicha September 22, 2023 19:08

Updated changelog

2adff54

michael-wb requested a review from ironage September 22, 2023 19:49

danieltabacaru reviewed Sep 22, 2023

View reviewed changes

Don't call handler functions more than once, esp for timers; updated …

a21588d

…documentation

danieltabacaru approved these changes Sep 22, 2023

View reviewed changes

Updated some comments

0705a8b

nhachicha approved these changes Sep 25, 2023

View reviewed changes

Michael Wilkerson-Barker added 2 commits September 25, 2023 12:49

Merge branch 'master' of github.com:realm/realm-core into mwb/fix-cap…

5377b6f

…itimer-double-delete

Updated changelog after release

b84ede8

michael-wb merged commit a9c8f99 into master Sep 25, 2023

michael-wb deleted the mwb/fix-capitimer-double-delete branch September 25, 2023 18:02

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michael-wb commented Sep 21, 2023 •

edited

Loading

coveralls-official bot commented Sep 21, 2023 •

edited

Loading

nhachicha left a comment

nhachicha Sep 22, 2023

nhachicha Sep 22, 2023

michael-wb Sep 22, 2023

nhachicha Sep 22, 2023

danieltabacaru Sep 22, 2023

danieltabacaru Sep 22, 2023

michael-wb Sep 22, 2023

michael-wb Sep 22, 2023

danieltabacaru Sep 22, 2023

michael-wb Sep 22, 2023

danieltabacaru Sep 22, 2023 •

edited

Loading

michael-wb Sep 22, 2023

danieltabacaru Sep 22, 2023

danieltabacaru Sep 22, 2023

danieltabacaru left a comment

michael-wb commented Sep 22, 2023

nhachicha left a comment

	* To be called when an error occurs - the actual error value with be provided when the websocket_closed
	* To be called when an error occurs - the actual error value will be provided when the websocket_closed

	m_timer_cancel(m_userdata, m_timer);
	m_timer_free(m_userdata, m_timer);

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

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

Conversation

michael-wb commented Sep 21, 2023 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented Sep 21, 2023 • edited Loading

Pull Request Test Coverage Report for Build michael.wilkersonbarker_855

💛 - Coveralls

nhachicha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

michael-wb commented Sep 22, 2023

nhachicha left a comment

Choose a reason for hiding this comment

michael-wb commented Sep 21, 2023 •

edited

Loading

coveralls-official bot commented Sep 21, 2023 •

edited

Loading

danieltabacaru Sep 22, 2023 •

edited

Loading