RCORE-2004 Make Realm::call_completion_callbacks() resilient to trasaction being deleted

[jedelbo]

What, How & Why?

Fixes #7434
I was not able to reproduce the problem, so mayby we are not getting the full picture. But this change will at lease prevent a nullpointer exception if the transaction is terminated.

☑️ ToDos

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

[coveralls-official]

Pull Request Test Coverage Report for Build jorgen.edelbo_175

Details

• 60 of 60 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 91.871%

---

Totals
Change from base Build 2191:	0.02%
Covered Lines:	244012
Relevant Lines:	265602

---

💛 - Coveralls

[tgoyne]

The sequence of events that'd hit this is that the async write completes, schedules the call to run_async_completions(), and then close() or invalidate() is called before the scheduler gets to run. It seems like it should be possible to test this (maybe with a custom Scheduler implementation that closes the Realm when the completion handler is scheduled?).

It might be better to change the early return to if (m_is_running_async_commit_completions || m_async_commit_q.empty()) {? do_invalidate() takes care of flushing the queue before closing the transaction, and if the queue is ever non-empty with no transaction then things are in a broken state.

[jedelbo]

@tgoyne I had the same idea as you, but realized that calling close() or invalidate() already flushes the callback-queue (through do_invalidate()).

[jedelbo]

Calling do_invalidate() from call_completion_callback() will lead to recursion.

[tgoyne]

I didn't say anything about calling do_invalidate() from call_completion_callback()?

[jedelbo]

I didn't say anything about calling do_invalidate() from call_completion_callback()?

Ok, but then I did not understand your suggestion. I just think that as long as we don't fully understand what leads up to this exception, then it is better to just fix the symptom.

[jedelbo]

@tgoyne I am not sure what the next step here should be.

[tgoyne]

It could use a test.

[jedelbo]

It could use a test.

@tgoyne So could I. The problem is that I can't create a test that reproduces this (as explained before). Can you see other ways that m_transaction can be set to nullptr other than through do_invalidate?

[tgoyne]

This can be reproduced with a custom scheduler that lets us invoke the callback at the correct time:

struct ManualScheduler : util::Scheduler {
    std::mutex mutex;
    std::condition_variable cv;
    std::vector<util::UniqueFunction<void()>> callbacks;

    void invoke(util::UniqueFunction<void()>&& cb) override
    {
        {
            std::lock_guard lock(mutex);
            callbacks.push_back(std::move(cb));
        }
        cv.notify_all();
    }

    bool is_on_thread() const noexcept override
    {
        return true;
    }
    bool is_same_as(const Scheduler*) const noexcept override
    {
        return false;
    }
    bool can_invoke() const noexcept override
    {
        return true;
    }
};
auto scheduler = std::make_shared<ManualScheduler>();
TestFile config;
config.schema_version = 0;
config.schema = Schema{{"object", {{"value", PropertyType::Int}}}};
config.scheduler = scheduler;
config.automatic_change_notifications = false;
auto realm = Realm::get_shared_realm(config);
realm->begin_transaction();
realm->async_commit_transaction([](std::exception_ptr) {});

std::vector<util::UniqueFunction<void()>> callbacks;
{
    std::unique_lock lock(scheduler->mutex);
    scheduler->cv.wait(lock, [&] {
        return !scheduler->callbacks.empty();
    });
    realm->close();
    callbacks.swap(scheduler->callbacks);
}
for (auto& cb : callbacks)
    cb();

[jedelbo]

@tgoyne Thank you very much for your help here. My mind was kind of trapped. I can now see that I misread your suggestion for a fix. I totally agree that that is the right solution.