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

RCOCOA-2301 Remove the emulated InterprocessMutex File cache #7448

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Mar 11, 2024

This was added to reduce the number of file descriptors used back when we opened a separate SharedGroup on each thread. We now normally only ever open one DB per file per process at a time, so sharing files between them is no longer useful and it's been the source of a few bugs.

Fixes realm/realm-swift#8507 by just removing the code containing the assertion entirely.

@tgoyne tgoyne self-assigned this Mar 11, 2024
@cla-bot cla-bot bot added the cla: yes label Mar 11, 2024
Copy link

coveralls-official bot commented Mar 11, 2024

Pull Request Test Coverage Report for Build thomas.goyne_254

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 92.03%

Totals Coverage Status
Change from base Build 2176: 0.02%
Covered Lines: 244702
Relevant Lines: 265893

💛 - Coveralls

@kiburtse
Copy link
Contributor

How would close affect this? Assertion suggests that the uid for the path has changed somehow. Locking of the static is pretty straight forward here, so something is very fishy. This refactoring is nice, but I feel like it's hiding probably the real problem. Please, add at least the same strong check into mapping where we also use get_file_uid. @ironage what do you think?

@kiburtse kiburtse requested a review from ironage March 13, 2024 22:48
@tgoyne
Copy link
Member Author

tgoyne commented Mar 14, 2024

My hypothesis is that the assertion is failing due to the map having an expired shared_ptr, not due to the path changing. This code should be deleted regardless of if it's correct or not, so I don't really see a reason to try to figure out what exactly is broken.

@tgoyne
Copy link
Member Author

tgoyne commented Mar 14, 2024

I think the uses of get_unique_id() in the encryption code should probably go away too.

@tgoyne tgoyne force-pushed the tg/interprocess-mutex-file-lock branch from 87b238a to e58e658 Compare March 14, 2024 16:45
@ironage
Copy link
Contributor

ironage commented Mar 14, 2024

Removing all uses of get_unique_id() sounds like a good idea to me. Issues like #7162 have broken my trust in it and I wouldn't be surprised if there is some other corner case causing issues. Maybe we can just use the file path instead?

@tgoyne
Copy link
Member Author

tgoyne commented Mar 15, 2024

Almost everything does just use the file path and if you break things by deleting a file while it's open and creating a new file at the same path, well that's your problem to deal with. The encryption stuff originally used the inode for performance reasons and not correctness, but those global maps also have no reason to exist any more. They were global because that's the only way to get data to signal handlers, and thankfully we no longer use signal handlers.

Copy link
Contributor

@ironage ironage left a comment

Choose a reason for hiding this comment

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

Nice reduction in complexity. LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
This was added to reduce the number of file descriptors used back when we
opened a separate SharedGroup on each thread. We now normally only ever open
one DB per file per process at a time, so sharing files between them is no
longer useful.
@tgoyne tgoyne force-pushed the tg/interprocess-mutex-file-lock branch from e58e658 to 1fe358d Compare March 29, 2024 15:59
@tgoyne tgoyne merged commit 6b07299 into master Mar 29, 2024
36 of 38 checks passed
@tgoyne tgoyne deleted the tg/interprocess-mutex-file-lock branch March 29, 2024 17:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 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.

Crash - Assertion failed: m_lock_info && m_lock_info->m_file.get_path() == m_filename
3 participants