RCORE-2092 Simplify the SessionWrapper lifecycle a bit #7609
Conversation
| // IMPORTANT: If a function is supplied that handles the exception, it must | ||
| // call abort() or cause the application to crash since the SyncClient will | ||
| // be in a bad state if this occurs and will not be able to shut down properly. |
There was a problem hiding this comment.
This comment was once true but was very out of date.
| @@ -44,47 +29,35 @@ using ProxyConfig = SyncConfig::ProxyConfig; | |||
|
|
|||
| // Life cycle states of a session wrapper: | |||
| // | |||
| // - Uninitiated | |||
There was a problem hiding this comment.
This comment was significantly out of date and not all the changes in the rewritten version are specifically related to this PR.
| // session wrapper (initiate()) will always find that `m_actualized` is | ||
| // true. This is the case, because the scheduling of such a post handler | ||
| // will have been preceded by the triggering of | ||
| // `ClientImpl::m_actualize_and_finalize` (in |
There was a problem hiding this comment.
The specific details of where in ClientImpl these things happen had gotten stale and I didn't think it was worth trying to describe here.
| try { | ||
| for (auto& p : m_server_slots) { | ||
| ServerSlot& slot = p.second; | ||
| if (m_one_connection_per_session) { | ||
| REALM_ASSERT(!slot.connection); | ||
| for (const auto& p : slot.alt_connections) { | ||
| ClientImpl::Connection& conn = *p.second; | ||
| if (conn.get_state() == ConnectionState::disconnected) { |
There was a problem hiding this comment.
I pushed this check into voluntary_disconnect().
| post([this](Status status) mutable { | ||
| if (status == ErrorCodes::OperationAborted) | ||
| return; | ||
| else if (!status.is_ok()) | ||
| throw Exception(status); |
There was a problem hiding this comment.
These checks appeared a lot, so I added a helper function rather than repeat them every time.
|
|
||
| /// The reason this synchronization session is used for. | ||
| /// Set a handler to monitor the state of download and upload progress. |
There was a problem hiding this comment.
These docs are stale (they should have been updated in the flx progress project) and I just moved them unchanged.
| state_wait_for(lock, State::Stopped); | ||
| m_thread.join(); |
There was a problem hiding this comment.
Restarting a socket provider didn't work due to missing join() here, as it's required to reassign a std::thread even if the old thread happens to have exited.
| m_thread = std::thread{&DefaultSocketProvider::event_loop, this}; | ||
| // Wait for the thread to start before continuing | ||
| state_wait_for(lock, State::Running); | ||
| } | ||
|
|
||
| void DefaultSocketProvider::OnlyForTesting::run_event_loop_on_current_thread(DefaultSocketProvider* provider) |
There was a problem hiding this comment.
This is now done via an observer instead.
| @@ -2009,21 +2011,10 @@ class Service::PostOper : public PostOperBase { | |||
| // Recycle the operation object before the handler is exceuted, such | |||
| // that the memory is available for a new post operation that might be | |||
| // initiated during the execution of the handler. | |||
| bool was_recycled = false; | |||
There was a problem hiding this comment.
All this error handling was just for the possibility of a handler that isn't nothrow moveable, which it turns out we don't need.
| @@ -328,25 +324,22 @@ class ClientImpl { | |||
| // approach would be to allow for per-endpoint SSL parameters to be | |||
| // specifiable through public member functions of ClientImpl from where they | |||
| // could then be picked up as new connections are created on demand. | |||
| // | |||
| // FIXME: `session_multiplex_ident` should be eliminated from ServerEndpoint | |||
There was a problem hiding this comment.
This isn't completely not an issue with user_id in ServerEndpoint, but it'd require very strange circumstances.
There was a problem hiding this comment.
i'm not even sure what this meant.
There was a problem hiding this comment.
If we set one_connection_per_session = false and then assigned each session a unique session_multiplex_ident we'd end up with a separate connection per session but tracked somewhat differently from when one_connection_per_session = true (one ServerSlot per connection rather than multiple connections in one ServerSlot), and this resulted in reconnect delays behaving differently..
tgoyne
changed the title
| @@ -328,25 +324,22 @@ class ClientImpl { | |||
| // approach would be to allow for per-endpoint SSL parameters to be | |||
| // specifiable through public member functions of ClientImpl from where they | |||
| // could then be picked up as new connections are created on demand. | |||
| // | |||
| // FIXME: `session_multiplex_ident` should be eliminated from ServerEndpoint | |||
There was a problem hiding this comment.
i'm not even sure what this meant.
|
|
||
| drain_connections_on_loop(); | ||
| } | ||
|
|
||
|
|
||
| void ClientImpl::register_unactualized_session_wrapper(SessionWrapper* wrapper, ServerEndpoint endpoint) |
There was a problem hiding this comment.
I actually had a branch I was working on in the past few days that did this same change as part of figuring out some base url stuff. 👍
tgoyne
force-pushed
the
tg/session-lifecycle
branch
2 times, most recently
from
d199a08 to
db9a599
Compare
It was both virtual and stored callbacks, which was redundant. Subclasses can store their own callbacks if desired (as the C API implementation already did).
tgoyne
force-pushed
the
tg/session-lifecycle
branch
2 times, most recently
from
56919c2 to
3425497
Compare
Initializing a sync::Session was a multi-step process of creating the Session with a config, configuring some addition things via mutation functions, and then calling bind(). We can simplify this a bit by pushing everything into the config struct and binding inside the wrapper's constructor, eliminating the constructed-but-not-initialized state and letting us make more of the members const.
tgoyne
force-pushed
the
tg/session-lifecycle
branch
from
3425497 to
8a68a7f
Compare
Some preparatory refactoring before adding a bunch of complexity around handling not being able to acquire the sync agent.
Initializing a sync::Session was a multi-step process of creating the Session with a config, configuring some additional things via mutation functions, and then calling bind(). Over time we've gradually reduced how many things were set via mutation functions or parameters to bind(), and now there was only two left. Both of them can easily be pushed to the Config struct, so the explicit bind() step is no longer needed and SessionWrapper can initialize immediately, eliminating one of the states that it can be in and letting us make more of the members const.