Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose sync session shutdown and realm delete files in one API #5646

Closed
wants to merge 5 commits into from

Conversation

nicola-cab
Copy link
Member

@nicola-cab nicola-cab commented Jul 5, 2022

What, How & Why?

Expose new C API for closing realm sync session and deleting files.
Fixes: #5542

☑️ ToDos

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

Copy link
Member Author

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @fealebenpae was against exposing this in the C API, but it was requested by @cmelchior. I believe this is what it is needed in order to expose this functionality in the C API.

Copy link
Member

@fealebenpae fealebenpae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against having this functionality in the C API, or at all, I think it can solve a lot of problems, but the current implementation is broken - SyncSession::shutdown_and_wait() will block until all sync sessions are terminated, or the sync client is stopped explicitly. For apps with more than one realm open at a time this means that the call will block until all other realms are closed and their sessions terminate, and that's not what our users would expect out of this API.

src/realm.h Outdated Show resolved Hide resolved
@nicola-cab
Copy link
Member Author

OK, I see what you mean. I don't see an easy solution, though. You can either block until all the sessions are closed or signal that all the sessions must close without waiting, but in this case deleting the files won't succeed ...

@fealebenpae
Copy link
Member

We could investigate fixing the SyncSession implementation to do what it advertises instead.

@nicola-cab
Copy link
Member Author

Adding @danieltabacaru in order to understand how complex would it be to fix this API before to expose it in the C API

@nicola-cab
Copy link
Member Author

We could investigate fixing the SyncSession implementation to do what it advertises instead.

What will this fix do? This is not clear to me. You don't want the client to block until all the sessions are closed? Doing this will for sure make realm_delete_fails fail or in any case the files won't be deleted if some other app is holding a valid handle/s to the file/s

@danieltabacaru
Copy link
Collaborator

danieltabacaru commented Jul 6, 2022

@nicola-cab @fealebenpae I looked at the code in SyncSession::shutdown_and_wait() and changing it does not seem straightforward. I agree the proposed fix will mostly be effective if only one session/realm is used by the app, otherwise the new API call may block indefinitely.

The sync client team has plans to refactor the code around SyncManager/SyncSession/SyncUser/etc and we could fix this then, so I guess the question is how urgent is this needed?

Bottom line, I don't see how this fix improves on using realm_app_sync_client_wait_for_sessions_to_terminate instead.

If we decide to go with this fix though, since we wait to close all sessions we should also delete all realms. So do something like realm_app_sync_client_wait_for_sessions_to_terminate + delete all realms.

@nicola-cab
Copy link
Member Author

realm_app_sync_client_wait_for_sessions_to_terminate

I would say we need this to allow SDKs to unlink successfully realm files on Windows. All the SDKs that run on Windows have this problem of needing to manage a file handle that is still kept alive (also some object store test does have this issue and I had to disable it).

This can also be needed in case of failures during realm initialization. Most of the time, the SDKs will need to close and delete all the realm files.

My preference here would be to expose a C API (or in general an API) that will close all the sync sessions (releasing the handle) and delete the realm files explicitly, to be called by the SDK in case this is needed.

I will make this PR a 'draft' and wait for SyncSession::shutdown_and_wait() fix

@nicola-cab nicola-cab marked this pull request as draft July 6, 2022 14:01
@nicola-cab nicola-cab closed this Mar 1, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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