Instant writes do not wait for asynchronous operations to complete

[mattmccutchen]

Unfortunately, I think I found another fundamental problem to rival #63. :(

For instant writes, the Transact.prototype.{insert,update,remove} methods and their helpers initiate various asynchronous operations and proceed without waiting for the operations to complete. These operations include:

1. Operations that take a callback: mainly writes to the underlying collection.
2. Client-side collection writes without a callback, which are implicitly asynchronous (blame Meteor for introducing this pitfall in the name of making things easy). I think this mainly affects writes to the Transactions collection for client-side instant writes.

This means that the sequence of operations may not be at all what it appears from reading the code. In many cases, this may be mostly harmless, but it's extremely difficult to reason about.

The problem doesn't affect server code that performs instant writes without a callback, with one exception I noticed: instant update passes a callback to the underlying write even if no callback was originally specified. Here's a test that demonstrates the problem in this case.

As long as we keep support for client-side instant writes, I think the only manageable solution would be to rewrite the affected methods using a promise library or using async/await (which would probably require transpilation to support runtimes that don't natively support async/await yet). Edit: We should just define Meteor methods for client-initiated instant writes. Then the instant write code will execute only on the server, so we can make the method bodies fully synchronous and have a wrapper for each method that implements the with-callback case in terms of the without-callback case (Meteor.defer + try/catch + call the callback).

[JackAdams]

"We should just define Meteor methods for client-initiated instant writes."

But what about the use case (which I actually use quite a lot*), where there is an instant update on the client, then a query on minimongo that could be affected by the outcome of the instant write, then further writes on the client are made depending on the query's result. That workflow, philosophically flawed though it is, wouldn't be possible any longer (and, in practice, it seems to work quite well).

* in an old app that does a lot of client side writes -- from back when I didn't know any better

One of the reasons that callbacks and error handling in this package are so crappy is that I pretty much never use either in my own code -- just trusting that everything will work and being prepared for a bit of manual db cleanup when it doesn't. So a lot of the code around error handling and callbacks is untested -- either by automated tests or by actual use in apps.

[mattmccutchen]

But what about the use case (which I actually use quite a lot*), where there is an instant update on the client, then a query on minimongo that could be affected by the outcome of the instant write, then further writes on the client are made depending on the query's result.

If the methods have stubs that simulate the writes on the client, then this should work the same way it does now (where the write to the underlying collection in _doInsert, etc. calls a predefined Meteor method that simulates the write on the client).

[JackAdams]

If the methods have stubs that simulate the writes on the client, then this should work the same way it does now (where the write to the underlying collection in _doInsert, etc. calls a predefined Meteor method that simulates the write on the client).

How would that differ from the current implementation? Aren't the client side mutations to minimongo sent to the server via methods already?

[mattmccutchen]

Currently, Transact.prototype.update (for example) does one or two writes to the Transactions collection in _recordTransaction plus the write to the original collection, and each write becomes a separate call to a predefined Meteor collection-write method. I realize now that the problem isn't as bad as I originally thought (which may explain why no one has complained): Meteor maintains the order of the method calls and each method call does a synchronous database write, but if one of the writes to the Transactions collection fails for some reason, the actual write will still be attempted. To avoid this problem under the current design, we'd have to break up update into multiple chunks using callbacks (ugly), promises, or async/await and verify the success of each write before starting the next one.

I'm proposing instead that we define a Meteor method whose server-side implementation does both the Transactions write(s) and the actual write, which would take the burden of sequencing these writes off of the client. (There may still be a need for the client to pass a callback that updates the local transaction manager state based on the result of the single method call -- I haven't thought this through in detail -- but this won't be as bad as what we'd need under the current design.)

[JackAdams]

Ah yes, I'd forgotten about the writes to the Transactions collection. That makes sense.