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

Use delete_files from core when deleting a Realm. #2401

Merged
merged 36 commits into from
Jun 29, 2021

Conversation

DominicFrei
Copy link
Contributor

@DominicFrei DominicFrei commented May 27, 2021

Description

This moves the DeleteRealm function into our Core since it's a shared functionality.
The Core PR enables all SDKs to use the same DeleteRealm instead of having duplicated and/or differing implementations of it across the SDKs.

Fixes #386
Fixes #2455

TODO

  • Changelog entry
  • Tests (if applicable)

Both not really necessary because the point of moving this does include not changing anything on the user API level.
The expectation would be to have all tests still pass even after deleting the function and just calling it's Core equivalent.

@DominicFrei DominicFrei self-assigned this May 27, 2021
Realm/Realm/Realm.cs Outdated Show resolved Hide resolved
@DominicFrei
Copy link
Contributor Author

Update on the status here: Core PR is merged, waiting for the v11 merge now.

@DominicFrei DominicFrei marked this pull request as ready for review June 4, 2021 12:43
@DominicFrei DominicFrei changed the title [WIP] Use delete_files from core when deleting a Realm. Use delete_files from core when deleting a Realm. Jun 4, 2021
@nirinchev
Copy link
Member

One thing I noticed as I was working on #2423 is that delete_files in Core will just go ahead and try to delete the files without first checking if the Realm exists:

https://github.com/realm/realm-core/blob/8765b60e5b4db7bcb4a14d4c22854f12e592b4ec/src/realm/object-store/shared_realm.cpp#L915-L916

It might be a good idea to do the check and perhaps do nothing if the Realm doesn't exist.

@DominicFrei
Copy link
Contributor Author

One thing I noticed as I was working on #2423 is that delete_files in Core will just go ahead and try to delete the files without first checking if the Realm exists:

https://github.com/realm/realm-core/blob/8765b60e5b4db7bcb4a14d4c22854f12e592b4ec/src/realm/object-store/shared_realm.cpp#L915-L916

It might be a good idea to do the check and perhaps do nothing if the Realm doesn't exist.

This is what happens at the moment. call_with_lock will successfully lock the lockfile (because opening seems to also create it if it does not exist) and then tries to delete all files which succeeds if they don't exist.

Delete on an empty folder will therefore return true as well == succeed without doing anything.

Would that suffice your intention? @nirinchev

@DominicFrei
Copy link
Contributor Author

One thing I noticed as I was working on #2423 is that delete_files in Core will just go ahead and try to delete the files without first checking if the Realm exists:
https://github.com/realm/realm-core/blob/8765b60e5b4db7bcb4a14d4c22854f12e592b4ec/src/realm/object-store/shared_realm.cpp#L915-L916
It might be a good idea to do the check and perhaps do nothing if the Realm doesn't exist.

This is what happens at the moment. call_with_lock will successfully lock the lockfile (because opening seems to also create it if it does not exist) and then tries to delete all files which succeeds if they don't exist.

Delete on an empty folder will therefore return true as well == succeed without doing anything.

Would that suffice your intention? @nirinchev

@nirinchev Added with 3dc9251. It does not fail locally for me though. Let's see what CI thinks about it. 💪

@DominicFrei DominicFrei requested a review from nirinchev June 8, 2021 17:00
@DominicFrei DominicFrei requested a review from nirinchev June 9, 2021 10:58
@DominicFrei DominicFrei added the Blocked This issue is blocked by another issue label Jun 10, 2021
@DominicFrei DominicFrei removed the Blocked This issue is blocked by another issue label Jun 10, 2021
@DominicFrei
Copy link
Contributor Author

DominicFrei commented Jun 11, 2021

Summary of the Android crash analysis so far:
https://ci.realm.io/blue/organizations/jenkins/realm%2Frealm-dotnet/detail/PR-2401/14/pipeline/445 shows that the process crashes during the Xamarin Android tests (the last successful test being SynchronizedInstanceTests.GetInstance_WhenDynamic_ReadsSchemaFromDisk right before the crash).

The android-logcat.zip (https://ci.realm.io/blue/organizations/jenkins/realm%2Frealm-dotnet/detail/PR-2401/14/artifacts) mentioned in the crash message shows:

06-10 17:36:06.097 E/libc++abi(13473): terminating with uncaught exception of type
 realm::util::File::NotFound: open("/data/data/io.realm.xamarintests/cache/rt-$13473/mongodb-realm/
 myapp-123/8e98563c-c188-43d2-9812-49bcd53b28dc/
 s_fe4873eb-028d-4fbf-bcbe-2ec70155ff4a.realm.management/access_control.write.mx") 
failed: No such file or directory 
Path: /data/data/io.realm.xamarintests/cache/rt-$13473/mongodb-realm/
 myapp-123/8e98563c-c188-43d2-9812-49bcd53b28dc/
 s_fe4873eb-028d-4fbf-bcbe-2ec70155ff4a.realm.management/access_control.write.mx

access_control.write.mx seems to be the write access mutex created in https://github.com/realm/realm-core/blob/master/src/realm/db.cpp#L951 (also see https://github.com/realm/realm-core/blob/master/src/realm/db.cpp#L770).

It is then used in https://github.com/realm/realm-core/blob/master/src/realm/db.cpp#L1774 and https://github.com/realm/realm-core/blob/master/src/realm/db.cpp#L2046.

It's unclear what exactly causes the crash. It does seem to only happen in this PR which is based on realm/realm-core#4701.

@DominicFrei
Copy link
Contributor Author

@nirinchev Jenkins is finally happy. 👍 Ready for review again.

Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Looks good, mostly nits from me.

Realm/Realm/Realm.cs Outdated Show resolved Hide resolved
Realm/Realm/Exceptions/RealmInUseException.cs Outdated Show resolved Hide resolved
Realm/Realm/Handles/SharedRealmHandle.cs Outdated Show resolved Hide resolved
Realm/Realm/Realm.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/SynchronizedInstanceTests.cs Outdated Show resolved Hide resolved
@DominicFrei DominicFrei requested a review from nirinchev June 16, 2021 10:02
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Once the File.Exists check is moved to OS, this should be good to go.

Tests/Realm.Tests/Sync/SynchronizedInstanceTests.cs Outdated Show resolved Hide resolved
Tests/Realm.Tests/Sync/SyncTestBase.cs Outdated Show resolved Hide resolved
Realm/Realm/Realm.cs Outdated Show resolved Hide resolved
Dominic Frei and others added 2 commits June 16, 2021 13:34
Co-authored-by: Nikola Irinchev <irinchev@me.com>
Co-authored-by: Nikola Irinchev <irinchev@me.com>
@DominicFrei DominicFrei requested a review from nirinchev June 29, 2021 13:13
@DominicFrei DominicFrei merged commit bffe1be into master Jun 29, 2021
@DominicFrei DominicFrei deleted the df/use-delete-realm-from-core branch June 29, 2021 14:09
nirinchev added a commit that referenced this pull request Jul 6, 2021
* master:
  Update the AppConfiguration.Logger obsolete message (#2495)
  Add more serializer types to the preserved ones (#2489)
  Update changelog from Core (#2488)
  Build an xcframework for iOS (#2475)
  Prepare for vNext (#2487)
  Prepare for 10.2.1 (#2484)
  Fix TableKey / ObjKey conversion on Android x86 (#2477)
  Update the build manager locations (#2474)
  Clean up native resources on Unity application exit (#2467)
  Fix some native warnings (#2483)
  Don't try to get the app from a removed user (#2480)
  Use delete_files from core when deleting a Realm. (#2401)
  Fixes for the sync datatype tests (#2461)
  Clean up the package.json and add some docs (#2452)
  Run iOS tests on CI (#2405)
  Verify that objects do not belong to different realm when added to collection (#2465)
  Updated README instructions (#2459)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeleteRealm wallways say file locked Move DeleteRealm into object store
2 participants