Added sync socket result enum for sync socket callback handlers in C API #7015

michael-wb · 2023-09-29T20:23:28Z

What, How & Why?

Added the realm_sync_socket_callback_result_e enum to be used for passing to the callback handler functions in the C API Platform Networking interface. This contains a subset of the realm_errno_e enum values that can be more easily used with this interface and simplifies this part of the interface, since it narrows down the set of possible errors that should be provided to the callback handlers.

NOTE: Additional errors were included that were believed to be applicable from the Platform Networking point of view, if they can be detected by the implementation. For the most part, the primary values used will be RLM_ERR_SYNC_SOCKET_SUCCESS or RLM_ERR_SYNC_SOCKET_OPERATION_ABORTED.

Fixes #7014

☑️ ToDos

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

coveralls-official · 2023-09-29T21:14:01Z

Pull Request Test Coverage Report for Build michael.wilkersonbarker_904

111 of 150 (74.0%) changed or added relevant lines in 8 files are covered.
61 unchanged lines in 16 files lost coverage.
Overall coverage increased (+0.05%) to 91.614%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/realm/object-store/c_api/types.hpp	8	10	80.0%
test/object-store/c_api/c_api.cpp	52	57	91.23%
src/realm/object-store/c_api/socket_provider.cpp	17	25	68.0%
src/realm/sync/noinst/client_impl_base.cpp	2	10	20.0%
src/realm/sync/network/default_socket.cpp	14	30	46.67%

Files with Coverage Reduction	New Missed Lines	%
src/realm/index_string.cpp	1	88.09%
test/test_table.cpp	1	99.64%
src/realm/array_string.cpp	2	87.78%
src/realm/cluster.cpp	2	80.25%
src/realm/object-store/c_api/socket_provider.cpp	2	74.48%
src/realm/object-store/shared_realm.cpp	2	93.74%
src/realm/util/serializer.cpp	2	90.91%
test/object-store/c_api/c_api.cpp	2	99.45%
src/realm/sort_descriptor.cpp	3	93.7%
test/test_thread.cpp	3	66.67%

Totals
Change from base Build 1771:	0.05%
Covered Lines:	230731
Relevant Lines:	251852

💛 - Coveralls

…llback_error_type

nhachicha

LGTM 🎉 minor comments

nhachicha · 2023-10-03T13:02:49Z

src/realm/error_codes.h

+typedef enum realm_sync_socket_callback_result {
+    // These error values are pulled directly from realm_errno_e
+    RLM_ERR_SYNC_SOCKET_SUCCESS = RLM_ERR_NONE,
+    RLM_ERR_SYNC_SOCKET_OPERATION_ABORTED = RLM_ERR_OPERATION_ABORTED,


FWIW we're only using the two above.

nhachicha · 2023-10-03T13:08:10Z

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

-static void realm_sync_socket_op_complete(realm_sync_socket_callback* realm_callback, realm_errno_e status,
-                                          const char* reason)
+static void realm_sync_socket_op_complete(realm_sync_socket_callback* realm_callback,
+                                          realm_sync_socket_callback_result_e result, const char* reason)
 {
    if (realm_callback->get() != nullptr) {


Since we moved the realm_callback release inside this method, is there a scenario where realm_callback->get() == nullptr? In fact, a double invocation (by error) of realm_sync_socket_op_complete will SIGSEGV before invoking get() since the realm_callback pointer was previously released.

We technically don't need it anymore - I'll remove it

danieltabacaru · 2023-10-03T13:29:30Z

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

@@ -15,8 +15,7 @@ struct CAPITimer : sync::SyncSocketProvider::Timer {
              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)


what prompted this change?

We were allowing the CAPITimer to delete the handler, but really it should have been "owned" by the implementation and freed when the complete/cancelled function was called, similar to the post and write_async callbacks.

I think the idea was that if none of the functions is called and the timer is deallocated, it then deallocates the handler too. Is that still possible?

We have to call either realm_sync_socket_timer_canceled or realm_sync_socket_timer_complete which free the handler otherwise the syn client hangs indefinitely waiting for the posted timer(s) to complete https://github.com/realm/realm-core/blob/master/src/realm/sync/client.cpp#L477-L478
This is why we should not rely on the dtor to free it.

Another issue with the current approach is that the m_timer_cancel(m_userdata, m_timer) runs asynchronously whereas realm_release(m_handler); below runs synchronously which will delete the FunctionHandler before it gets a chance to be used inside the event loop

danieltabacaru · 2023-10-05T15:27:57Z

src/realm/error_codes.h

+    RLM_ERR_SYNC_SOCKET_SUCCESS = RLM_ERR_NONE,
+    RLM_ERR_SYNC_SOCKET_OPERATION_ABORTED = RLM_ERR_OPERATION_ABORTED,
+    RLM_ERR_SYNC_SOCKET_RUNTIME = RLM_ERR_RUNTIME,
+    RLM_ERR_SYNC_SOCKET_OUT_OF_MEMORY = RLM_ERR_OUT_OF_MEMORY,


Are we sure we want/need users provide some of these errors? (i.e, RLM_ERR_SYNC_SOCKET_OUT_OF_MEMORY)

No, but they could potentially be returned via the async write callback.

test/object-store/c_api/c_api.cpp

danieltabacaru · 2023-10-05T16:06:07Z

test/object-store/c_api/c_api.cpp

    };

    struct TestWebSocket : realm::c_api::WrapC, WebSocketInterface {
    public:
-        TestWebSocket(DefaultSocketProvider& socket_provider, realm_websocket_endpoint_t endpoint,
+        TestWebSocket(DefaultSocketProvider* socket_provider, realm_websocket_endpoint_t endpoint,


not sure why we want this change

Thanks - reverted

danieltabacaru

LGTM - make sure to fix the leak 👍

…llback_error_type

…() callbacks

…llback_error_type

nhachicha

Tested it again, CI is happy 👍

…llback_error_type

michael-wb · 2023-10-19T20:43:30Z

The three failures in the following test run were due to unrelated timeouts during the test. Created issue 7071 to investigate these failures:
https://evergreen.mongodb.com/version/65315ea632f417d05a36f1c3?redirect_spruce_users=true

danieltabacaru · 2023-10-20T08:24:53Z

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

@@ -116,7 +107,7 @@ struct CAPIWebSocket : sync::WebSocketInterface {
    }

    /// Destroys the web socket instance.
-    ~CAPIWebSocket()
+    virtual ~CAPIWebSocket()


doesn't need to be virtual

danieltabacaru · 2023-10-20T10:22:41Z

src/realm.h

@@ -355,35 +355,70 @@ typedef struct realm_websocket_endpoint {
    bool is_ssl;            // true if SSL should be used
 } realm_websocket_endpoint_t;

+// The following definitions are for internal structures and pointers used


It seems the SDKs use them too. I am not a big fan splitting it this way, but I don't have a strong opinion either. People directly using this API can have their say.

…llback_error_type

Added sync socket result enum for sync socket callback handlers in C API

4688246

michael-wb self-assigned this Sep 29, 2023

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

Updated changelog

cca806a

michael-wb requested review from nhachicha and danieltabacaru September 29, 2023 21:15

Michael Wilkerson-Barker added 4 commits October 2, 2023 16:07

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

c5e8698

…llback_error_type

CAPI: timer callbacks are now released by canceled/complete function

fa8045c

Updated c_api tests to use all sync socket c_api functions

4f7a8d6

Additional updates to sync socket c api test

9fe0653

nhachicha approved these changes Oct 3, 2023

View reviewed changes

danieltabacaru reviewed Oct 3, 2023

View reviewed changes

danieltabacaru reviewed Oct 5, 2023

View reviewed changes

test/object-store/c_api/c_api.cpp Show resolved Hide resolved

danieltabacaru reviewed Oct 5, 2023

View reviewed changes

danieltabacaru approved these changes Oct 5, 2023

View reviewed changes

nhachicha mentioned this pull request Oct 11, 2023

Platform networking realm/realm-kotlin#1528

Merged

3 tasks

Michael Wilkerson-Barker added 8 commits October 11, 2023 15:39

Added CAPI write callback manager to manage async write callbacks

a4ad166

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

efc8ac3

…llback_error_type

Pass error codes up to default socket provider for async_write_binary…

2d2d7ff

…() callbacks

Removed async write callback manager from CAPI

c16494e

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

255728a

…llback_error_type

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

fa36650

…llback_error_type

Updated changelog after release

79b92d4

clang format and updates from review

c37fc37

michael-wb requested review from danieltabacaru and nhachicha October 13, 2023 20:03

Michael Wilkerson-Barker added 2 commits October 16, 2023 10:47

Update async_write_binary() error handling to not throw exception

5bc581d

Updated a few comments

fe43d49

Another comment update

b33f7e6

nhachicha approved these changes Oct 17, 2023

View reviewed changes

danieltabacaru requested a review from fealebenpae October 19, 2023 10:19

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

7b79f74

…llback_error_type

danieltabacaru reviewed Oct 20, 2023

View reviewed changes

danieltabacaru approved these changes Oct 20, 2023

View reviewed changes

fealebenpae approved these changes Oct 20, 2023

View reviewed changes

Michael Wilkerson-Barker added 2 commits October 20, 2023 13:03

Updates from review

8527c76

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

9ca5090

…llback_error_type

michael-wb merged commit 43853eb into master Oct 20, 2023
2 checks passed

michael-wb deleted the mwb/capi_callback_error_type branch October 20, 2023 19:34

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

Added sync socket result enum for sync socket callback handlers in C API #7015

Added sync socket result enum for sync socket callback handlers in C API #7015

michael-wb commented Sep 29, 2023 •

edited

Loading

coveralls-official bot commented Sep 29, 2023 •

edited

Loading

nhachicha left a comment

nhachicha Oct 3, 2023

nhachicha Oct 3, 2023

michael-wb Oct 3, 2023

danieltabacaru Oct 3, 2023

michael-wb Oct 3, 2023

danieltabacaru Oct 3, 2023

nhachicha Oct 4, 2023 •

edited

Loading

danieltabacaru Oct 5, 2023

michael-wb Oct 13, 2023

danieltabacaru Oct 5, 2023

michael-wb Oct 19, 2023

danieltabacaru left a comment

nhachicha left a comment

michael-wb commented Oct 19, 2023 •

edited

Loading

danieltabacaru Oct 20, 2023

danieltabacaru Oct 20, 2023

Added sync socket result enum for sync socket callback handlers in C API #7015

Added sync socket result enum for sync socket callback handlers in C API #7015

Conversation

michael-wb commented Sep 29, 2023 • edited Loading

What, How & Why?

☑️ ToDos

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

Pull Request Test Coverage Report for Build michael.wilkersonbarker_904

💛 - 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

nhachicha Oct 4, 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

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

nhachicha left a comment

Choose a reason for hiding this comment

michael-wb commented Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Sep 29, 2023 •

edited

Loading

coveralls-official bot commented Sep 29, 2023 •

edited

Loading

nhachicha Oct 4, 2023 •

edited

Loading

michael-wb commented Oct 19, 2023 •

edited

Loading