Update BindingCallbackThreadObserver to be one per sync client #6156

michael-wb · 2022-12-30T03:11:30Z

What, How & Why?

The BindingCallbackThreadObserver was originally a global instance that was shared across all sync client/default socket provider instances. The changes in this PR move the BindingCallbackThreadObserver to be a shared_ptr parameter in the SyncClientConfig, which allows for either a single instance to be shared across all sync clients or a unique instance per sync client. The observer class was also updated to allow optional implementations for the three functions, instead of a base class that required all three functions to be implemented.

In addition, a C API test was added for the BindingCallbackThreadObserver to verify its operation (and some updates to the interface were needed).

NOTE: The BindingCallbackThreadObserver is only used by the default sync socket provider implementation. It is up to custom sync socket provider implementations to add their own support for threaded callbacks.

Fixes #6250

☑️ ToDos

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

… sync test

danieltabacaru

CI reports a data race in Sync_MultipleSyncAgentsNotAllowed. LGTM otherwise

danieltabacaru · 2023-01-27T07:53:48Z

There are some bootstrap tests failing. Are they related to these changes?

michael-wb · 2023-01-27T13:52:02Z

There are some bootstrap tests failing. Are they related to these changes?

I believe these (and the data race) failures are fixed in Jonathans PRs, #6244 and #6216(already merged) - I will wait to merge until his changes are merged to verify these are fixed.

…oop-error-handler

michael-wb · 2023-01-28T05:01:15Z

There are some bootstrap tests failing. Are they related to these changes?

I believe these (and the data race) failures are fixed in Jonathans PRs, #6244 and #6216(already merged) - I will wait to merge until his changes are merged to verify these are fixed.

Turns out it was a valid data race - I updated the test similar to how Jonathan updated other tests in test_sync.cpp to define the DefaultSocketProvider separately so stop() could be called manually. Now the test is passing.

jbreams · 2023-02-06T19:00:32Z

src/realm/sync/client_base.hpp

@@ -185,6 +186,10 @@ struct ClientConfig {
 // and creating WebSockets. If not provided the default implementation will be used.
 std::shared_ptr<sync::SyncSocketProvider> socket_provider;

+ // Optional thread observer for event loop thread events in the default SyncSocketProvider
+ // implementation. It is not used for custom SyncSocketProvider implementations.
+ std::shared_ptr<BindingCallbackThreadObserver> default_socket_provider_thread_observer;


does this really need shared lifetime management? could we just use a unique_ptr here?

It could technically just be a unique_ptr, since the SyncClientConfig is required for each start of the sync client. The main question is whether the SDK would prefer to use a single shared ptr across all the apps or is it fine to define a separate observer instance for each app. @cmelchior, any preferences?

jbreams · 2023-02-06T19:02:20Z

src/realm/sync/binding_callback_thread_observer.hpp

+ virtual ~BindingCallbackThreadObserver() = default;
+
+ // Set the global thread observer with the provided (optional) callback functions
+ static void set_global_thread_observer(std::unique_ptr<BindingCallbackThreadObserver>&& observer_ptr)


Do we really need to keep the old global API here? Currently this is used by just one SDK. @cmelchior, is adopting the new per-app API sufficiently complicated for java that we should support both APIs for now?

No, it shouldn't be complicated. Removing this is fine 👍

OK - I'll remove the global support and update the CAPI.

jbreams · 2023-02-06T19:03:15Z

src/realm/sync/network/default_socket.cpp

@@ -507,6 +510,10 @@ void DefaultSocketProvider::start()

 m_logger_ptr->trace("Default event loop: start()");
 REALM_ASSERT(m_state == State::Stopped);
+
+ // Reset the service so it can be run() again
+ m_service.reset();


Is this ever actually needed? Currently the default socket provider is only used once, except in testing.

It was only for completeness if the default socket provider ever needed to be restarted. I can remove it.

jbreams · 2023-02-06T19:52:33Z

I would say that we should start with unique ownership to start and see if we need shared ownership later. Lots fewer ways to screw up with unique ownership.

…

On Mon, Feb 6, 2023 at 2:42 PM Michael Wilkerson-Barker < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/realm/sync/client_base.hpp <#6156 (comment)>: > @@ -185,6 +186,10 @@ struct ClientConfig { // and creating WebSockets. If not provided the default implementation will be used. std::shared_ptr<sync::SyncSocketProvider> socket_provider; + // Optional thread observer for event loop thread events in the default SyncSocketProvider + // implementation. It is not used for custom SyncSocketProvider implementations. + std::shared_ptr<BindingCallbackThreadObserver> default_socket_provider_thread_observer; It could technically just be a unique_ptr, since the SyncClientConfig is required for each start of the sync client. The main question is whether the SDK would prefer to use a single shared ptr across all the apps or is it fine to define a separate observer instance for each app. @cmelchior <https://github.com/cmelchior>, any preferences? — Reply to this email directly, view it on GitHub <#6156 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABOVUMBPEZFBPAS4AT6QPLWWFHZHANCNFSM6AAAAAATMSEIKM> . You are receiving this because your review was requested.Message ID: ***@***.***>

…oop-error-handler

michael-wb · 2023-02-24T15:00:05Z

The description was updated - thanks

…

On Fri, Feb 24, 2023 at 7:20 AM Daniel Tabacaru ***@***.***> wrote: I guess the description needs an update — Reply to this email directly, view it on GitHub <#6156 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZ2FARWSQF4PWCN7RCMC2YLWZCRP7ANCNFSM6AAAAAATMSEIKM> . You are receiving this because you were assigned.Message ID: ***@***.***>

…oop-error-handler

jbreams · 2023-02-24T15:26:15Z

src/realm.h

+ * @param user_data pointer to user defined data that is provided to each of the callback functions
+ * @param free_userdata callback invoked when the user_data is to be freed
+ */
+RLM_API void realm_sync_client_config_set_default_binding_thread_observer(


I thought we decided to just have the per-app version...

jbreams · 2023-02-24T15:36:06Z

src/realm/sync/binding_callback_thread_observer.cpp

+void BindingCallbackThreadObserver::call_will_destroy_thread(
+ const std::shared_ptr<BindingCallbackThreadObserver>& observer_ptr)
+{
+ // Call into the observer ptr if not null, otherwise, use the global thread observer


It looks like there is no global observer? Do we still need these static functions?

These were just helper functions... I'll remove them and just use the direct function calls in the default socket provider.

cla-bot bot added the cla: yes label Dec 30, 2022

michael-wb marked this pull request as ready for review December 30, 2022 21:02

sync-by-unito bot mentioned this pull request Jan 2, 2023

SyncSocketProvider eventloop migration #5448

Closed

michael-wb requested review from jbreams and danieltabacaru January 3, 2023 14:08

michael-wb linked an issue Jan 3, 2023 that may be closed by this pull request

Re-enable/fix the multiple sync agents sync test #6160

Closed

sync-by-unito bot mentioned this pull request Jan 3, 2023

Re-enable/fix the multiple sync agents sync test #6160

Closed

michael-wb self-assigned this Jan 3, 2023

michael-wb force-pushed the mwb/event-loop-migration branch 2 times, most recently from 992b676 to 1b738ce Compare January 13, 2023 06:52

michael-wb force-pushed the mwb/event-loop-migration branch 2 times, most recently from 3d1b750 to f49ac5b Compare January 19, 2023 13:46

michael-wb marked this pull request as draft January 20, 2023 21:03

michael-wb force-pushed the mwb/event-loop-migration branch from f5d2ff2 to d6abd48 Compare January 25, 2023 19:59

Base automatically changed from mwb/event-loop-migration to master January 26, 2023 00:03

Michael Wilkerson-Barker added 3 commits January 26, 2023 15:31

Added test and updated binding callback c_api - updated commented out…

9297774

… sync test

REQUIRE() not REALM_ASSERT()

7c95e91

Update/Fix changelog

f60cc60

michael-wb force-pushed the mwb/event-loop-error-handler branch from 70af2c6 to f60cc60 Compare January 26, 2023 20:55

Michael Wilkerson-Barker added 2 commits January 26, 2023 15:56

clang format

019fe2a

Allow nullptr userdata

1b69942

michael-wb marked this pull request as ready for review January 26, 2023 21:36

jbreams approved these changes Jan 26, 2023

View reviewed changes

danieltabacaru approved these changes Jan 27, 2023

View reviewed changes

Michael Wilkerson-Barker added 3 commits January 27, 2023 20:44

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

bfdcceb

…oop-error-handler

Revert changelog

c6e7b59

Reworked event loop in test to fix TSAN failure

3261f7f

jbreams reviewed Feb 6, 2023

View reviewed changes

michael-wb mentioned this pull request Feb 6, 2023

[C API] Integration test for websocket provider #6277

Merged

3 tasks

Michael Wilkerson-Barker added 4 commits February 20, 2023 21:24

Removed global binding callback thread observer

b9a1cf0

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

555e12d

…oop-error-handler

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

0a9812a

…oop-error-handler

sync tests are no longer hanging

4567abd

michael-wb requested review from danieltabacaru and jbreams February 22, 2023 20:55

michael-wb changed the title ~~Add event loop error handler for testing~~ Update BindingCallbackThreadObserver so it is one per sync client Feb 24, 2023

michael-wb removed a link to an issue Feb 24, 2023

Re-enable/fix the multiple sync agents sync test #6160

Closed

Michael Wilkerson-Barker added 3 commits February 23, 2023 21:21

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

6d2f826

…oop-error-handler

Reverted sync client changes and disabled MultipleSyncAgents test

a2c1241

Removed service.reset()

d8cb9d0

michael-wb changed the title ~~Update BindingCallbackThreadObserver so it is one per sync client~~ Update BindingCallbackThreadObserver to be one per sync client Feb 24, 2023

sync-by-unito bot mentioned this pull request Feb 24, 2023

Add per app support for BindingCallbackThreadObserver #6250

Closed

Michael Wilkerson-Barker added 2 commits February 24, 2023 10:04

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

b73d863

…oop-error-handler

Updated changelog after merging release

b972b54

jbreams reviewed Feb 24, 2023

View reviewed changes

danieltabacaru approved these changes Feb 24, 2023

View reviewed changes

Michael Wilkerson-Barker added 4 commits February 24, 2023 12:41

Cleaned up binding callback observer interface and c_api

047a9b6

Additional thread observer cleanup

4f39bd2

removed unused header files

0e92873

Found one more unused header file

552a97c

jbreams approved these changes Feb 24, 2023

View reviewed changes

michael-wb merged commit 09381ba into master Feb 24, 2023

michael-wb deleted the mwb/event-loop-error-handler branch February 24, 2023 18:59

kiburtse mentioned this pull request Mar 3, 2023

Merge master, adapt exceptions #6361

Merged

3 tasks

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

Update BindingCallbackThreadObserver to be one per sync client #6156

Update BindingCallbackThreadObserver to be one per sync client #6156

michael-wb commented Dec 30, 2022 •

edited

Loading

danieltabacaru left a comment

danieltabacaru commented Jan 27, 2023

michael-wb commented Jan 27, 2023

michael-wb commented Jan 28, 2023

jbreams Feb 6, 2023

michael-wb Feb 6, 2023

jbreams Feb 6, 2023

cmelchior Feb 6, 2023

michael-wb Feb 6, 2023

jbreams Feb 6, 2023

michael-wb Feb 6, 2023

jbreams commented Feb 6, 2023 via email

michael-wb commented Feb 24, 2023 via email

jbreams Feb 24, 2023

jbreams Feb 24, 2023

michael-wb Feb 24, 2023

Update BindingCallbackThreadObserver to be one per sync client #6156

Update BindingCallbackThreadObserver to be one per sync client #6156

Conversation

michael-wb commented Dec 30, 2022 • edited Loading

What, How & Why?

☑️ ToDos

danieltabacaru left a comment

Choose a reason for hiding this comment

danieltabacaru commented Jan 27, 2023

michael-wb commented Jan 27, 2023

michael-wb commented Jan 28, 2023

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

jbreams commented Feb 6, 2023 via email

michael-wb commented Feb 24, 2023 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Dec 30, 2022 •

edited

Loading