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 use of http transport from background isolate #1470

Merged

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented Jan 11, 2024

After the introduction of realm app caching hot-restart and background isolates no longer works reliably with realm.

Fixes: #1467
Fixes: #1466
Fixes: #1451
Fixes: #1433 (only half fixed by #1445)

Copy link

coveralls-official bot commented Jan 11, 2024

Pull Request Test Coverage Report for Build 7612377268

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 87.323%

Totals Coverage Status
Change from base Build 7555413703: 0.2%
Covered Lines: 3706
Relevant Lines: 4244

💛 - Coveralls

@nielsenko nielsenko force-pushed the kn/fix-use-of-http_transport-from-background-isolate-crash branch from 26d7f81 to 9376ced Compare January 12, 2024 09:48
@nielsenko nielsenko added the no-changelog Used to skip the changelog check label Jan 15, 2024
@nielsenko
Copy link
Contributor Author

The issues fixed in this PR was introduced with , but was never released, so I didn't update the CHANGELOG.

Since Apps are now cached and shared we need to be vigilant to always route to the correct isolate in cases such as App.logIn as we potentially need to do the request in another isolate than the one calling, and hence handle async native callbacks in both.

Hence we need to ensure all data is available not only when the callback is scheduled, but also when it actually runs. The current realm C-api expects callback to be executed synchronously, so we need to deep-copy all non-value arguments and handle lifetime explicitly. This involves a bit of simple and dumb native code, such as:

RLM_API void realm_dart_user_completion_callback(realm_userdata_t userdata, realm_user_t* user, const realm_app_error_t* error)
{
    // we need to make a deep copy of error, because the message pointer points to stack memory
    auto error_copy = realm_app_error_copy(error);

    // take an extra ref to the user, so that it doesn't get deleted before we invoke the callback
    std::shared_ptr<realm::SyncUser> user_copy; 
    if (user != nullptr) {
        user_copy = realm_user(*user);
    }

    auto ud = reinterpret_cast<realm_dart_userdata_async_t>(userdata);
    ud->scheduler->invoke([ud, error = std::move(error_copy), user = realm_user(user_copy)]() mutable { 
        (reinterpret_cast<realm_app_user_completion_func_t>(ud->dart_callback))(ud->handle, &user, error.get());
    });
}

Going forward we could:

  1. Re-work app caching in core to be better suited for Darts memory model,
  2. Create Dart specific C-api with the new binding generator introduced for realm-js, that is geared towards async callbacks,
  3. Wait for [vm/ffi] Introduce a NativeCallable.blocking dart-lang/sdk#54554 (comment), which should allow us to get rid of these trampolines, or
  4. Just keep adding these trampolines as needed.

BTW: Note that (unrelated to the fix) the Dart native interface has been bumped to 2.2, which is backwards compatible with 2.0 that we used previously.

@nielsenko nielsenko marked this pull request as ready for review January 15, 2024 08:47
@nielsenko nielsenko force-pushed the kn/fix-use-of-http_transport-from-background-isolate-crash branch 3 times, most recently from 880329d to 501d614 Compare January 16, 2024 08:38
@nielsenko nielsenko force-pushed the kn/fix-use-of-http_transport-from-background-isolate-crash branch from 015fc7e to 2c0de6c Compare January 22, 2024 12:54
@nielsenko nielsenko force-pushed the kn/fix-use-of-http_transport-from-background-isolate-crash branch from 8df35e1 to 453fe79 Compare January 22, 2024 14:07
@nielsenko nielsenko merged commit 7a69fb2 into main Jan 22, 2024
42 of 46 checks passed
@nielsenko nielsenko deleted the kn/fix-use-of-http_transport-from-background-isolate-crash branch January 22, 2024 14:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-changelog Used to skip the changelog check
Projects
None yet
1 participant