Need way to close a synchronized Realm and all underlying file resources

[cmelchior]

This is a slightly different use case than https://github.com/realm/realm-core/issues/5364

All SDK's have a public method for deleting Realms: https://github.com/realm/realm-core/blob/master/src/realm/object-store/shared_realm.hpp#L421.

However, for synchronized Realms this method has race conditions (that in many cases) prevent it from being used correctly. The reason is that the SyncClient keeps a file reference open and there is no way for end-users to know when the SyncClient releases those.

https://github.com/realm/realm-core/blob/master/src/realm/object-store/sync/sync_manager.hpp#L157 is a work-around that work-ish for tests, but isn't really useful for end-users as they might have multiple Realms opened.

Ideally, ObjectStore also exposes a way to fully close all file resources owned by Core/ObjectStore so users can delete the file. If a blocking call is hard to do, even having a closeAsync(), with an event firing on the SyncClient thread would be good enough as we can wrap it in the SDK layer.

Suggested API's:

// Variant 1, if true, will block until any SyncSession is also fully closed.
SharedRealm::close(bool terminateSession = true);

// Variant 2, will trigger callback when public and SyncSession Realm is closed.
SharedRealm::closeAsync(CloseCallback callback);

[nirinchev]

If you're using SessionStopPolicy::Immediately and you're not holding references to sync sessions, the resources should be released as soon as you close the realm file.

[nicola-cab]

Isn't this already covered by this? https://github.com/realm/realm-core/pull/5540 We miss:

1. Exposure in the C API
2. Async version.

[cmelchior]

@nirinchev

If you're using SessionStopPolicy::Immediately and you're not holding references to sync sessions, the resources should be released as soon as you close the realm file.

Are you sure that is true? My understanding was that since the SyncClient is operating on its own thread. You cannot release resources that way? Especially since SessionStopPolicy::Immediately isn't really "immediate".

@nicola-cab Not exactly, #5540 does most of what we want, true, but there are other API's that require that the Realm isn't opened by anyone and if people cannot rely on a close() method to actually close the Realm it is causing issues.

[nirinchev]

Are you sure that is true?

Almost. So based on what we've seen in dart and .NET, calling SharedRealm::close and releasing all shared_ptr-s for the sync session does result in the file being closed and delete-able. The only issues we've seen so far are with Dart on Windows, where the delete may fail because "something" is holding the file open and we need to retry. In the case of Windows, it doesn't appear to be an issue with Realm itself because we manage to acquire the lock and start deleting stuff (i.e. Realm believes there's no one using the file). So there's still some race there, but probably outside of our control. This is not an issue on Unix platforms because unlinking a file that is in use there is fine.

The question is whether you need this for public use (i.e. an API you want to expose to end users) or for tests. If it's for tests, then setting the stop policy to immediate and retrying the deletion on Windows is probably "good enough". If it's to expose to end users so that they can delete their Realms, that's probably not a good idea (we don't even expose the stop policy in .NET and Dart). OS already exposes a SyncSession::shutdown_and_wait, so we can probably expose that via the C API fairly easily without the need to add an extra argument to SharedRealm::close().

[cmelchior]

Ah, didn't realise there was a SyncSession::shutdown_and_wait(). That looks like that might due the trick, but yes, that is needed in the C-API.

[fealebenpae]

I advise against using SyncSession::shutdown_and_wait() because that blocks until all sync sessions are closed and the sync client is shut down.

[nicola-cab]

OK so it seems we have 2 different issues:

1. close and delete for good everything if init data callback fails (we don't need to expose this). I think that a failure in the init callback during the initialization phase, that takes a little longer to close, makes sense. Also, because we are not inside a critical path.

2. Expose for the C-API consumers SyncSession::shutdown_and_wait() , however, it seems there is some disagreement..

[cmelchior]

A few modifications:

1. Is also unrelated to initialization. I agree that Core should do it automatically if initialization fails, but SDK's also need a manual method for doing it.

2. The important use case is that we need a method that can close a Realm and release file locks. If that can be done through SyncSession::shutdown_and_wait that is fine, but isn't a strict requirement.

[sync-by-unito[bot]]

➤ Nicola Cabiddu commented:

In order to expose this functionality, we need a proper fix in:

SyncSession::shutdown_and_wait()

that the sync team should prioritize. [[email protected]] [[email protected]] [[email protected]]

[sync-by-unito[bot]]

➤ nicola-cab commented:

In order to implement this, we require support from the Sync team in order to fix {}SyncSession::shutdown_and_wait{}.

[danieltabacaru]

I advise against using SyncSession::shutdown_and_wait() because that blocks until all sync sessions are closed and the sync client is shut down.

@fealebenpae I don't think that's the case. shutdown_and_wait blocks until a specific task is run on the event loop (at which point finalize() would have been called on the abandoned session(s)).

[fealebenpae]

@danieltabacaru SyncSession::shutdown_and_wait() calls SyncClient::wait_for_session_terminations(), which in turn finally calls sync::Client::wait_for_session_terminations_or_client_stopped(), which implies that waiting for one particular session to shut down actually waits for other sessions to shut down. The issue is that we need a way to close a realm and wait until its sync session has also released the file on disk - were we to implement it using SyncSession::shutdown_and_wait() as originally discussed here we'll be blocked until other sessions are terminated, not just the particular one we're interested in. Looking more closely at the comment on sync::Client::wait_for_session_terminations_or_client_stopped() in realm/sync/client.hpp it appears we're just giving an opportunity for the event loop to run so it can release the file and perform other final housekeeping or returning immediately if the client is stopped and the event loop and associated resources are already freed, right? This leads me to believe this isn't the API we want in any case as it can noticeably block the UI thread. Instead we might want to add a callback to SyncSession which is asynchronously ran when the underlying session is fully terminated on the sync client's event loop and implement the Realm::closeAsync(CloseCallback callback) method @cmelchior proposed originally.

EDIT: @danieltabacaru you're right and I was wrong - after following the SyncClient::wait_for_session_terminations() rabbit hole I agree that my original statement you quoted is incorrect. However I still think this API is unsuitable because of the potential to noticeably block the UI thread.

[danieltabacaru]

@fealebenpae I agree using shutdown_and_wait may not be the best API. I am not sure we need to wait for the session to be fully terminated (makes the implementation a bit more complicated too), I think releasing the file is enough (could be I'm missing something)

[fealebenpae]

@danieltabacaru that’s correct - from the SDK perspective we just need to know when the file is released so we can move or delete it. We don’t particularly care of any other housekeeping tasks that happen.