Wait for sessions to drain during sync client shutdown #6293

jbreams · 2023-02-10T02:52:07Z

What, How & Why?

There's a race when force closing a connection where sessions may still be getting finalized. This makes it so that if there are still any active sessions on a connection, force_close() makes it so that the connection will close as soon as the last session has ended and will skip all the linger period timing.

☑️ ToDos

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

danieltabacaru · 2023-02-10T15:05:34Z

src/realm/sync/noinst/client_impl_base.hpp

@@ -1264,6 +1266,12 @@ inline void ClientImpl::Connection::one_less_active_unsuspended_session()
 {
    if (--m_num_active_unsuspended_sessions != 0)
        return;
+
+    if (m_force_closed) {


should the check also include m_state != ConnectionState::disconnected?

jbreams · 2023-02-10T15:58:38Z

If this goes into the current release then it doesn't need a changelog entry since the assertions its fixing haven't been released yet.

michael-wb

LGTM - I didn't see the new debug statements in any of the test runs I checked, but the logic looks good.

danieltabacaru · 2023-02-10T17:39:22Z

There is a failure in CI:
ERROR in Sync_FailingReadsOnServerSide: CHECK(compare_groups(rt_1, rt_2))

I think it should not be related to this change.

jbreams · 2023-02-11T21:34:43Z

The test failures were actually relevant. I've gone and re-worked some things and added more asserts. I've run the all the realm-sync-tests locally 1000 times with the latest changes.

michael-wb

These changes look good - just a few minor questions/comments

michael-wb · 2023-02-12T16:34:25Z

src/realm/sync/noinst/client_impl_base.cpp

@@ -544,6 +568,11 @@ void Connection::initiate_reconnect_wait()
    REALM_ASSERT(!m_reconnect_delay_in_progress);
    REALM_ASSERT(!m_disconnect_delay_in_progress);

+    // If we've been force closed then we don't need/want to reconnect. Just return early here.
+    if (m_force_closed) {
+        return;


Should this also ensure the m_reconnect_disconnect_timer timer is stopped (e.g. m_reconnect_disconnect_timer.reset()), or maybe that should still happen in Connection::force_close()

Yes, I've also added some assertions to make sure that we end up disconnected at the end of voluntary_disconnect and that we're in the correct delay mode if we are disconnected.

michael-wb · 2023-02-12T16:41:09Z

src/realm/sync/client.cpp

    {
        std::lock_guard lock{m_mutex};
        m_actualize_and_finalize_needed = false;
        swap(m_unactualized_session_wrappers, unactualized_session_wrappers);
        swap(m_abandoned_session_wrappers, abandoned_session_wrappers);
+        stopped = m_stopped;


Do you need this since stopped is now an atomic? You can read m_stopped directly on line 602 and avoid a tiny window where m_stopped might be changed between here and there.

I decided not to make m_stopped atomic after all. I think since we're threading force_closed states down into sessions now, we can just use that to efficiently check if it's safe to call callbacks etc rather than introducing an atomic bool.

michael-wb · 2023-02-12T16:56:50Z

src/realm/sync/client.cpp

@@ -491,24 +495,17 @@ void ClientImpl::drain_connections_on_loop()
 {
    post([this](Status status) mutable {
        REALM_ASSERT(status.is_ok());
-        actualize_and_finalize_session_wrappers();
        drain_connections();
    });
 }

 void ClientImpl::drain()


What do you think about renaming Client::drain() to something like Client::shutdown() or Client::shutdown_and_wait() so the name is a bit more descriptive?

Good idea! I was actually thinking the same thing but just hadn't typed it up yet.

…async

jbreams · 2023-02-13T19:32:33Z

src/realm/sync/client.cpp

@@ -491,24 +495,17 @@ void ClientImpl::drain_connections_on_loop()
 {
    post([this](Status status) mutable {
        REALM_ASSERT(status.is_ok());
-        actualize_and_finalize_session_wrappers();
        drain_connections();
    });
 }

 void ClientImpl::drain()


Good idea! I was actually thinking the same thing but just hadn't typed it up yet.

jbreams · 2023-02-15T15:48:23Z

src/realm/sync/client.cpp

    {
        std::lock_guard lock{m_mutex};
        m_actualize_and_finalize_needed = false;
        swap(m_unactualized_session_wrappers, unactualized_session_wrappers);
        swap(m_abandoned_session_wrappers, abandoned_session_wrappers);
+        stopped = m_stopped;


I decided not to make m_stopped atomic after all. I think since we're threading force_closed states down into sessions now, we can just use that to efficiently check if it's safe to call callbacks etc rather than introducing an atomic bool.

jbreams · 2023-02-15T15:58:41Z

test/sync_fixtures.hpp

@@ -566,7 +566,9 @@ class MultiClientServerFixture {
    {
        unit_test::TestContext& test_context = m_test_context;
        stop();
-        m_clients.clear();
+        for (int i = 0; i < m_num_clients; ++i) {
+            m_clients[i]->shutdown_and_wait();


it turns out there can be callbacks that may fire during the call to shutdown_and_wait() and that if we clear this array than the unique_ptr in m_clients will be null and callbacks will segfault. So we need to defer destroying the Clients until they are actually all shutdown.

jbreams · 2023-02-15T18:35:08Z

src/realm/sync/noinst/client_impl_base.cpp

        m_conn.one_less_active_unsuspended_session(); // Throws
+        on_suspended(SessionErrorInfo{error.code(), false});


This was actually a bug where we'd end up making the number of active unsuspended sessions underflow.

jbreams · 2023-02-15T18:36:06Z

src/realm/sync/noinst/client_impl_base.hpp

@@ -1262,8 +1266,10 @@ inline void ClientImpl::Connection::one_more_active_unsuspended_session()

 inline void ClientImpl::Connection::one_less_active_unsuspended_session()
 {
+    REALM_ASSERT(m_num_active_unsuspended_sessions);


I've added these assertions wherever we -- a variable because there was at least one case where we were underflowing and making this value uint64_t(-1).

I like having the assertions there - it will help with tracking down any issues in the future.

jbreams · 2023-02-15T18:36:46Z

src/realm/sync/noinst/client_impl_base.cpp

@@ -544,6 +568,11 @@ void Connection::initiate_reconnect_wait()
    REALM_ASSERT(!m_reconnect_delay_in_progress);
    REALM_ASSERT(!m_disconnect_delay_in_progress);

+    // If we've been force closed then we don't need/want to reconnect. Just return early here.
+    if (m_force_closed) {
+        return;


Yes, I've also added some assertions to make sure that we end up disconnected at the end of voluntary_disconnect and that we're in the correct delay mode if we are disconnected.

michael-wb

Looks good - I walked through a couple of shutdown scenarios and I couldn't come up with any possible issues that aren't already handled. One area that I thought might me an issue was around the outstanding_posts, but the timers always run the handler when canceled and the code waits for all the posts to complete.

michael-wb · 2023-02-16T05:31:36Z

src/realm/sync/noinst/client_impl_base.hpp

@@ -1262,8 +1266,10 @@ inline void ClientImpl::Connection::one_more_active_unsuspended_session()

 inline void ClientImpl::Connection::one_less_active_unsuspended_session()
 {
+    REALM_ASSERT(m_num_active_unsuspended_sessions);


I like having the assertions there - it will help with tracking down any issues in the future.

jbreams · 2023-02-16T18:36:55Z

@danieltabacaru , do you want to weigh in here?

danieltabacaru · 2023-02-16T19:16:12Z

@danieltabacaru , do you want to weigh in here?

Yes, I'll have a look this evening.

danieltabacaru · 2023-02-16T21:43:43Z

src/realm/sync/noinst/client_impl_base.cpp

+    std::vector<Session*> to_close;
+    for (auto& session_pair : m_sessions) {
+        if (session_pair.second->m_state == Session::State::Active) {
+            to_close.push_back(session_pair.second.get());


nit: why not closing the sessions directly here?

jbreams · 2023-02-16T21:56:05Z

Because closing the session may remove the session from m_sessions and invalidate the iterators used to go through the list of sessions. So I copy the session pointers to close to a secondary vectors to ensure the iterators remain valid all the way through the loop.

…

On Thu, Feb 16, 2023 at 4:43 PM Daniel Tabacaru ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/realm/sync/noinst/client_impl_base.cpp <#6293 (comment)>: > } - REALM_ASSERT(m_num_active_unsuspended_sessions == 0); - REALM_ASSERT(m_num_active_sessions == 0); - if (m_state == ConnectionState::disconnected) { - return; + std::vector<Session*> to_close; + for (auto& session_pair : m_sessions) { + if (session_pair.second->m_state == Session::State::Active) { + to_close.push_back(session_pair.second.get()); nit: why not closing the sessions directly here? — Reply to this email directly, view it on GitHub <#6293 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABOVUMZK6L34K3MMYM4QWLWX2NRVANCNFSM6AAAAAAUXJJ4UI> . You are receiving this because you were assigned.Message ID: ***@***.***>

danieltabacaru

LGTM

…async

cla-bot bot added the cla: yes label Feb 10, 2023

github-actions bot assigned jbreams Feb 10, 2023

jbreams marked this pull request as ready for review February 10, 2023 15:00

jbreams requested review from danieltabacaru and michael-wb February 10, 2023 15:00

danieltabacaru reviewed Feb 10, 2023

View reviewed changes

jbreams requested a review from danieltabacaru February 10, 2023 15:45

jbreams added the no-changelog label Feb 10, 2023

michael-wb approved these changes Feb 10, 2023

View reviewed changes

danieltabacaru approved these changes Feb 10, 2023

View reviewed changes

jbreams requested review from danieltabacaru and michael-wb February 11, 2023 21:34

michael-wb reviewed Feb 12, 2023

View reviewed changes

jbreams added 6 commits February 15, 2023 10:46

Wait for sessions to drain during sync client shutdown

707814c

check m_state

158372e

fixes from debugging realm-sync-tests

8b00b3e

format

cf62e6b

fix issue where num_active_unsuspended_sessions falls below zero

b41acac

undo atomic bool and rename functions

7cfef56

jbreams force-pushed the jbr/drain_sessions_async branch from 8212db9 to 7cfef56 Compare February 15, 2023 15:46

jbreams added 3 commits February 15, 2023 10:53

fix compile

3373c48

Merge remote-tracking branch 'origin/master' into jbr/drain_sessions_…

e3ab51f

…async

also cancel timer in force close

2174e23

jbreams commented Feb 15, 2023

View reviewed changes

michael-wb approved these changes Feb 16, 2023

View reviewed changes

danieltabacaru reviewed Feb 16, 2023

View reviewed changes

danieltabacaru approved these changes Feb 16, 2023

View reviewed changes

jbreams added 3 commits February 16, 2023 17:41

add clarifyin comment

8bc49bb

Merge remote-tracking branch 'origin/master' into jbr/drain_sessions_…

39d8a81

…async

changelog

7451714

jbreams removed the no-changelog label Feb 16, 2023

jbreams merged commit b05f9f0 into master Feb 17, 2023

jbreams deleted the jbr/drain_sessions_async branch February 17, 2023 00:46

kiburtse mentioned this pull request Feb 20, 2023

Occasional tests "no_sessions" assertion failure in SyncManager #6271

Open

kiburtse mentioned this pull request Mar 3, 2023

Merge master, adapt exceptions #6361

Merged

3 tasks

kiburtse mentioned this pull request Jul 31, 2023

Data race on transaction commit after sync client shutdown #6844

Closed

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

Wait for sessions to drain during sync client shutdown #6293

Wait for sessions to drain during sync client shutdown #6293

jbreams commented Feb 10, 2023 •

edited

Loading

danieltabacaru Feb 10, 2023

jbreams Feb 10, 2023

jbreams commented Feb 10, 2023

michael-wb left a comment

danieltabacaru commented Feb 10, 2023

jbreams commented Feb 11, 2023

michael-wb left a comment

michael-wb Feb 12, 2023

jbreams Feb 15, 2023

michael-wb Feb 12, 2023

jbreams Feb 15, 2023

michael-wb Feb 12, 2023

jbreams Feb 13, 2023

jbreams Feb 13, 2023

jbreams Feb 15, 2023

jbreams Feb 15, 2023

jbreams Feb 15, 2023

jbreams Feb 15, 2023

michael-wb Feb 16, 2023

jbreams Feb 15, 2023

michael-wb left a comment

michael-wb Feb 16, 2023

jbreams commented Feb 16, 2023

danieltabacaru commented Feb 16, 2023

danieltabacaru Feb 16, 2023

jbreams commented Feb 16, 2023 via email

danieltabacaru left a comment

		m_conn.one_less_active_unsuspended_session(); // Throws
		on_suspended(SessionErrorInfo{error.code(), false});

Wait for sessions to drain during sync client shutdown #6293

Wait for sessions to drain during sync client shutdown #6293

Conversation

jbreams commented Feb 10, 2023 • edited Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbreams commented Feb 10, 2023

michael-wb left a comment

Choose a reason for hiding this comment

danieltabacaru commented Feb 10, 2023

jbreams commented Feb 11, 2023

michael-wb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbreams commented Feb 16, 2023

danieltabacaru commented Feb 16, 2023

Choose a reason for hiding this comment

jbreams commented Feb 16, 2023 via email

danieltabacaru left a comment

Choose a reason for hiding this comment

jbreams commented Feb 10, 2023 •

edited

Loading