Migrate service and event loop into DefaultSyncSocket #6151
Conversation
| @@ -1783,6 +1784,12 @@ void Client::stop() noexcept | |||
| } | |||
|
|
|||
|
|
|||
| void Client::post_for_testing(SyncSocketProvider::FunctionHandler&& handler) | |||
There was a problem hiding this comment.
One thing I just realized is that none of the new post functions (i.e, that use FunctionHandler as parameter) have the argument (Status) needed by the handlers when they are executed. I guess Service class does not complain because it's templated (and accepts pretty much everything) and it probably assigns a default Status for the handlers.
There was a problem hiding this comment.
It was actually updated network.hpp in PR #6116 to provide a Status back to the handler:
https://github.com/realm/realm-core/blob/master/src/realm/sync/network/network.hpp#L2018
Timers were updated as well:
https://github.com/realm/realm-core/blob/master/src/realm/sync/network/network.hpp#L3510
There was a problem hiding this comment.
Got it. But shouldn't the caller provide such a status? Or maybe I completely misunderstood its usage.
There was a problem hiding this comment.
The status value is meant to be provided by the event loop based on what happened while the handler was in the event loop queue and transitioning to executing. For now post() doesn't have a status other than OK, since posted handlers can't be canceled. Timer's on the other hand, can provide either OK or Aborted depending on whether or not the timer was canceled.
There was a problem hiding this comment.
I see. I thought websockets can report a status too since it's ErrorCodes we use.
There was a problem hiding this comment.
They will need to in the future... that's a later PR - for now, we're still using the old mechanism.
2a96592 to
b960dcc
Compare
992b676 to
1b738ce
Compare
5caea1c to
d73d7a9
Compare
|
The TSAN failure is due to the event loop thread calling do_log() while the test is setting the log level threshold value - this is expected to be fixed by PR #6198 |
3d1b750 to
f49ac5b
Compare
test/test_sync.cpp
Outdated
|
|
||
| // Wait for the progress to be called | ||
| progress_pf.future.get(); | ||
| lock.lock(); |
There was a problem hiding this comment.
Is this really needed? If not, you can keep the block scope above.
There was a problem hiding this comment.
I don't need these changes in the current state - I believe I was getting a failure before, but I'm doing things differently so it's not an issue anymore. However, I don't believe progress_handler() is being called since the client is torn down immediately after calling session->bind()
There was a problem hiding this comment.
I actually hit a race condition in one of the sync-tests where it was tearing down the client and the progress_handler was called, but the mutex had already been destroyed... I updated this test to use a cond_var and added a note that this will need to be updated once I update the DefaultSocketProvider to remove stop() from client.stop()
f5d2ff2 to
d6abd48
Compare
|
2 failures were due to the recurring "non-additive schema changes" error and the third is new where a request to the BaasAdminApi using curl timed out during |
What, How & Why?
Migrate the
network::Serviceobject fromClientImpltoDefaultSyncSocketand update operation so the event loop thread is no longer owned bySyncClient. Updaterealm-sync-teststo work with the new scheme.The
BindingCallbackThreadObserverclass and global variable was moved from src/realm/object-store to src/realm/sync.First set of updates for #5448
☑️ ToDos