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

Fix deadlock when accessing user instance from within a listener #7186

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

takameyer
Copy link
Contributor

@takameyer takameyer commented Dec 6, 2023

What, How & Why?

  • Remove lock before calling log_in
  • Add test for deadlock

Fixes #7183

☑️ ToDos

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

Copy link

coveralls-official bot commented Dec 6, 2023

Pull Request Test Coverage Report for Build andrew.meyer_4

  • 41 of 41 (100.0%) changed or added relevant lines in 2 files are covered.
  • 118 unchanged lines in 15 files lost coverage.
  • Overall coverage decreased (-0.02%) to 91.735%

Files with Coverage Reduction New Missed Lines %
src/realm/index_string.cpp 1 87.81%
src/realm/index_string.hpp 1 82.86%
src/realm/util/file_mapper.cpp 1 76.07%
test/test_dictionary.cpp 1 99.85%
test/test_index_string.cpp 1 94.13%
test/test_table.cpp 1 99.64%
src/realm/sync/noinst/server/server_history.cpp 2 67.94%
src/realm/sort_descriptor.cpp 3 93.7%
src/realm/sync/noinst/client_impl_base.cpp 3 85.5%
src/realm/sync/noinst/client_reset.cpp 4 92.96%
Totals Coverage Status
Change from base Build 1897: -0.02%
Covered Lines: 232187
Relevant Lines: 253106

💛 - Coveralls

Copy link
Member

@tgoyne tgoyne left a comment

Choose a reason for hiding this comment

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

Other than the style nit this looks fine. Just needs a changelog entry.

// m_current_user is normally set very indirectly via the metadata manger
if (!m_metadata_manager)
m_current_user = new_user;
auto user = std::shared_ptr<SyncUser>(nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

std::shared_ptr<SyncUser> user; is the normal way to write this.

@takameyer takameyer force-pushed the andrew/double-login branch from 5293314 to 82cb295 Compare December 7, 2023 08:03
@takameyer takameyer marked this pull request as ready for review December 7, 2023 08:03
@takameyer takameyer requested review from jedelbo and removed request for jbreams and michael-wb December 7, 2023 08:03
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

Small suggestion on the changelog entry.

Co-authored-by: Kræn Hansen <kraen.hansen@mongodb.com>
else { // LoggedOut => LoggedIn
auto user = *it;

// LoggedOut => LoggedIn
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what this comment means anymore? I guess we're in in get_user() which implies we don't have a logged in user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this case assumes a cached user is changing to a logged in state, but in the case I'm testing against, we are updating the current logged in users auth tokens.
Comment could probably be removed or adapted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say just remove it. I don't think it was adding much context.

Copy link
Member

Choose a reason for hiding this comment

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

There was originally comments like this for each of the User state transitions, but they weren't maintained as the code was modified/

@takameyer takameyer merged commit 9686dff into master Dec 7, 2023
34 checks passed
@takameyer takameyer deleted the andrew/double-login branch December 7, 2023 13:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 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.

Deadlock when calling App::current_user from a User listener
4 participants