RDART-1020: Fix writeAsync behaviour

[nielsenko]

This PR has been edited to simply disallow async callbacks in all cases, even to writeAsync.

---

Calling Realm.writeAsync with an async callback is currently broken, since the callback is not awaited, and not passed as a FutureOr<T> Function().

The first commit adds tests thats illustrates a number of issues with the current approach, and the second fixes them.

We could consider detecting re-entrancy, and either throw an exception, or simply allow it by participating in the calling transaction, but this is not handled by the current PR.

Fixes: #1667

[nirinchev]

I don't think we should do that. We're intentionally guiding people toward using the sync API because holding the write lock for extended periods of time is an antipattern and can result in app freezes in multithreaded/process scenarios and can usually be avoided by doing the async calls before entering the write transaction.

My suggestion would be to instead check if T is a future and log a warning or even error out.

[nielsenko]

I don't think we should do that. We're intentionally guiding people toward using the sync API because holding the write lock for extended periods of time is an antipattern and can result in app freezes in multithreaded/process scenarios and can usually be avoided by doing the async calls before entering the write transaction.

My suggestion would be to instead check if T is a future and log a warning or even error out.

I disagree.

We already expose beginWriteTransaction and friends, so users have all the foot-guns they need available already. writeAsync is just a 10-line wrapper around these.

If we force people to not use async callbacks in writeAsync they will just start using those APIs directly. I think it is much better to make writeAsync behave sane, and just keep warning against long running transactions. We can definitely ensure that writeAsync never blocks, as long as the callback don't.

We should however make such a check in write. Today you can pass an async callback unhindered to write, which affords no sane semantics.

[nirinchev]

Yes, I know it's not difficult to do it and I don't think we should be super actively preventing people from doing it, but I want to make it harder for them to shoot themselves in the foot. The writeAsync API is the default one people are likely to use, so if that has a future as its argument, it'll imply that we condone or even encourage doing async work in the callback. Instead, my preference would be to throw in case you pass in a future and if someone complains, explain they could achieve the same with the begin/commit API. Happy to bounce ideas about it at some point this week though as it's mostly a cautious preference rather than a strong conviction against it.

[nirinchev]

Why do we await here if we assert the callback isn't a future?

[nielsenko]

😄 ever vigilant. I could just drop it. As is, all that is needed to support async callbacks is to change the return type of writeCallback from T to FutureOr<T> .. and re-enable some skipped tests.

I should probably have put this PR back into in-progress

[nielsenko]

.. but it is not a bug per-se .. in dart you can await non-futures, even null just fine. It differs from .NET on that.

[nielsenko]

I have removed all the new tests again and simply disallowed async callbacks even to writeAsync.

[coveralls-official]

Pull Request Test Coverage Report for Build 9300668832

Details

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

---

Totals
Change from base Build 9300439599:	0.008%
Covered Lines:	5905
Relevant Lines:	6788

---

💛 - Coveralls