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.

Sign up for GitHub

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

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

Merged
merged 9 commits into from
Sep 11, 2023
Merged

Throw an exception if File::unlock has failed + use same logic for all platforms, for detecting when we hold a lock on a file #6926

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36b1885
throw an exception if File::unlock has failed
nicola-cab Aug 24, 2023
2174e62
code review
nicola-cab Aug 25, 2023
70d56f9
fix changelog
nicola-cab Aug 25, 2023
fb00459
merge master
nicola-cab Aug 28, 2023
7a71337
merge master
nicola-cab Aug 30, 2023
bc7edfb
use same logic for all platforms for locking and unlocking the file
nicola-cab Aug 31, 2023
a75e5d2
merge master
nicola-cab Sep 1, 2023
cca4eb1
code review
nicola-cab Sep 1, 2023
e55c5dd
Remove unneeded message, which may be confusing. (#6963)
kiburtse Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* Add a distinct error code for timeouts (SyncConnectTimeout) rather than using the same one as for less transient failures ([PR #6932](https://github.com/realm/realm-core/pull/6932)).
* Throw an exception if `File::unlock` has failed, in order to inform the SDK that we are likely hitting some limitation on the OS filesystem, instead of crashing the application and use the same file locking logic for all the platforms.([PR #6926](https://github.com/realm/realm-core/pull/6926))


### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
Expand Down
28 changes: 19 additions & 9 deletions src/realm/util/file.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,7 @@ void File::open_internal(const std::string& path, AccessMode a, CreateMode c, in
int fd = ::open(path.c_str(), flags2, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
if (0 <= fd) {
m_fd = fd;
m_have_lock = false;
if (success)
*success = true;
return;
Expand Down Expand Up @@ -562,7 +563,8 @@ void File::close() noexcept

if (m_fd < 0)
return;
unlock();
if (m_have_lock)
Copy link
Contributor

Choose a reason for hiding this comment

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

great! So that's what was the issue all along it seems.... We just don't need to lock at all for mentioned usecases in bug reports

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but bear in mind that is very likely that since the system call for unlocking the file fails on certain circumstances, the same system call will eventually fail also when locking the file. So an exception will be thrown either way.

unlock();
int r = ::close(m_fd);
REALM_ASSERT_RELEASE(r == 0);
m_fd = -1;
Expand Down Expand Up @@ -1098,7 +1100,16 @@ static void _unlock(int m_fd)
do {
r = flock(m_fd, LOCK_UN);
} while (r != 0 && errno == EINTR);
REALM_ASSERT_RELEASE_EX(r == 0 && "File::unlock()", r, errno);
if (r) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So almost all good now, i'm not entirely convinced about this bit still.

We've almost misdiagnosed this issues 9th time in a row, do i understand it right? Essentially, we tried to unlock what we didn't lock (most likely from bugreports) since it wasn't needed for Realm::convert (Realm.writeCopyTo in java/kotlin sdks).

And since the assumption was that unlock is noop if file is not locked, we called it unconditionally every time and it was fine until new storage restrictions in Android hit and this syscall started to fail. So we were crashing apps for no reason for few years already.

Under #5107 and realm-java#6893 it was already mentioned that users activated needed permissions, and it was still crashing. To me this case already looks irrelevant, since we're not going to hit exact same code path now. The message may still be relevant for some realm opening scenarios though. @cmelchior could you confirm this?

Also, it seems that sqlite3 essentially ignored the result of unlock call failure (although logs it still internally) on close at least. The same seems to be true for windows where in most cases the return result is ignored, and it never asserts or propages further.

Exception in this case not much better that assertion. @finnschiermer shouldn't we just dump the error into logger, but essentially ignore it? Why interrupt any flow here?
@nicola-cab please, it's been a long issue if we account for very first bug reports, could you justify why do you think it is still needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to throw an exception for 2 reasons.
a) it is just a much better user experience, image that you are developing your app and your app crashes without any apparent reason, what would you do? ... most probably open an issue in order to try to understand what's going on.

b) this is a limitation of some SDKs, but impacts also us, they don't easily print error messages that are coming from our ASSERTIONS. Yes, there are some ways for the developer to check the system logs, but I would not rely on that, and surely we can't grab those logs from the device ourselves.

Ignoring the error can be a viable option, but I don't think it is correct. IMO we need to merge this PR in our code.

std::string message =
"File::unlock() has failed, this is likely due to some limitation in the OS file system.";
#ifdef REALM_ANDROID
message +=
" The most common cause is trying to use Realm on external storage. From Android API 30 this is no "
"longer supported. See https://developer.android.com/about/versions/11/privacy/storage for more details.";
#endif
throw SystemError(errno, message);
}
}
#endif

Expand Down Expand Up @@ -1203,9 +1214,9 @@ bool File::rw_lock(bool exclusive, bool non_blocking)
bool File::lock(bool exclusive, bool non_blocking)
{
REALM_ASSERT_RELEASE(is_attached());
REALM_ASSERT_RELEASE(!m_have_lock);

#ifdef _WIN32 // Windows version
REALM_ASSERT_RELEASE(!m_have_lock);

// Under Windows a file lock must be explicitely released before
// the file is closed. It will eventually be released by the
Expand Down Expand Up @@ -1250,8 +1261,10 @@ bool File::lock(bool exclusive, bool non_blocking)
if (non_blocking)
operation |= LOCK_NB;
do {
if (flock(m_fd, operation) == 0)
if (flock(m_fd, operation) == 0) {
m_have_lock = true;
return true;
}
} while (errno == EINTR);
int err = errno; // Eliminate any risk of clobbering
if (err == EWOULDBLOCK)
Expand All @@ -1262,10 +1275,10 @@ bool File::lock(bool exclusive, bool non_blocking)

void File::unlock() noexcept
{
#ifdef _WIN32 // Windows version
if (!m_have_lock)
return;

#ifdef _WIN32 // Windows version
OVERLAPPED overlapped;
overlapped.hEvent = 0;
overlapped.OffsetHigh = 0;
Expand All @@ -1276,11 +1289,8 @@ void File::unlock() noexcept
REALM_ASSERT_RELEASE(r);
m_have_lock = false;
#else
// The Linux man page for flock() does not state explicitely that
// unlocking is idempotent, however, we will assume it since there
// is no mention of the error that would be reported if a
// non-locked file were unlocked.
_unlock(m_fd);
m_have_lock = false;
nicola-cab marked this conversation as resolved.
Show resolved Hide resolved
#endif
}

Expand Down
8 changes: 7 additions & 1 deletion src/realm/util/file.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,9 +627,9 @@ class File {
class Streambuf;

private:
bool m_have_lock = false; // Only valid when m_fd is not null
#ifdef _WIN32
HANDLE m_fd = nullptr;
bool m_have_lock = false; // Only valid when m_fd is not null
#else
int m_fd = -1;
#ifdef REALM_FILELOCK_EMULATION
Expand Down Expand Up @@ -983,9 +983,12 @@ inline File::File(File&& f) noexcept
#ifdef _WIN32
m_fd = f.m_fd;
m_have_lock = f.m_have_lock;
f.m_have_lock = false;
f.m_fd = nullptr;
#else
m_fd = f.m_fd;
m_have_lock = f.m_have_lock;
nicola-cab marked this conversation as resolved.
Show resolved Hide resolved
f.m_have_lock = false;
#ifdef REALM_FILELOCK_EMULATION
m_pipe_fd = f.m_pipe_fd;
m_has_exclusive_lock = f.m_has_exclusive_lock;
Expand All @@ -1003,10 +1006,13 @@ inline File& File::operator=(File&& f) noexcept
#ifdef _WIN32
m_fd = f.m_fd;
m_have_lock = f.m_have_lock;
f.m_have_lock = false;
f.m_fd = nullptr;
#else
m_fd = f.m_fd;
f.m_fd = -1;
m_have_lock = f.m_have_lock;
nicola-cab marked this conversation as resolved.
Show resolved Hide resolved
f.m_have_lock = false;
#ifdef REALM_FILELOCK_EMULATION
m_pipe_fd = f.m_pipe_fd;
f.m_pipe_fd = -1;
Expand Down