Catch synchronous errors in Doc._otApply
#259
Conversation
1 similar comment
lib/client/doc.js
Outdated
| @@ -889,7 +900,7 @@ Doc.prototype._hardRollback = function(err) { | |||
| this.fetch(function() { | |||
| var called = op && callEach(op.callbacks, err); | |||
| for (var i = 0; i < pending.length; i++) { | |||
| callEach(pending[i].callbacks, err); | |||
| called = callEach(pending[i].callbacks, err) || called; | |||
There was a problem hiding this comment.
@nateps @ericyhwang do you know if this change will have any weird side-effects? Originally it would not count a pending callback as having called something, so it would both call back, and emit, which seems a bit odd?
It's forced some updates to a couple of edge case tests, which I'll highlight also. I'm assuming they're okay to change...? In them, we actually assert that the callback is called and an error is emitted; is that the expected behaviour?
alecgibson
force-pushed
the
otapply-rollback
branch
from
9484d7f to
9e15df4
Compare
|
Also apologies for the code coverage decrease, but I don't have any idea how to reach the other
|
Fixes #257 As outlined in the above issue, calling `Doc.submitOp` can sometimes `throw` a synchronous error, for example when submitting an invalid op that causes `type.apply` to `throw`. This is surprising behaviour, because an error handler should already be provided in the callback supplied to `submitOp`. What's more, the doc could be left partially mutated and out-of-sync with the server, which could lead to confusing behaviour. This change wraps all internal usage of `Doc._otApply` in `try`/`catch` blocks. If an error is thrown in this method, then we attempt to recover with `Doc._hardRollback`, which will attempt to reset the document and then call the callbacks of the pending ops with the error. This change also prevents `Doc._hardRollback` from emitting an error if at least one pending op callback was invoked, so that we don't branch error handling behaviour.
alecgibson
force-pushed
the
otapply-rollback
branch
from
9e15df4 to
d41e034
Compare
|
Okay - code coverage back up I think. I did actually manage to work out some crazy contrived (but theoretically possible) way that |
|
As discussed, we should check that every op has a callback. If any of them don't, then we should emit an error (so that no errors are ever swallowed). |
We previously followed the following logic for callbacks in a hard
rollback:
- check there's an inflight op, and that it has a callback
- if this isn't the case, then emit the error
This logic doesn't work very well in the situation where we don't have
an inflight op. This can happen in the following case:
- I submit an invalid op
- the op is added to the pending ops queue
- we attempt to apply the op to the local document before flushing
- `type.apply` throws, so the op is never flushed from pending to
inflight
- we perform a hard rollback
- we now find we have no inflight op, but we do have a pending op
In this case, since we have no inflight op, we'd emit the error, but
also call the pending op callback, causing both a callback call _and_
an error emission, which is a bit surprising.
This change updates our logic, so that:
- we check that there are some ops to callback
- and that all of those ops have callbacks
If there are no callbacks, or if any of the ops don't handle the error,
then we emit, so that we can be sure of never swallowing an error. We
also avoid treating the inflight op any differently to pending ops, so
that we avoid behaviour that's dependent on where in its lifecycle an op
throws.
However, if the client handles all of their op submission errors, then
we never emit.
|
@ericyhwang @nateps I've updated the hard rollback callback logic. I've actually tweaked it a tiny bit more than discussed - the inflight op is no longer treated as "special" relative to other pending ops. Here's the commit message for clarity: We previously followed the following logic for callbacks in a hard
This logic doesn't work very well in the situation where we don't have
In this case, since we have no inflight op, we'd emit the error, but This change updates our logic, so that:
If there are no callbacks, or if any of the ops don't handle the error, However, if the client handles all of their op submission errors, then |
Fixes #257
As outlined in the above issue, calling
Doc.submitOpcan sometimesthrowa synchronous error, for example when submitting an invalid opthat causes
type.applytothrow.This is surprising behaviour, because an error handler should already be
provided in the callback supplied to
submitOp. What's more, the doccould be left partially mutated and out-of-sync with the server, which
could lead to confusing behaviour.
This change wraps all internal usage of
Doc._otApplyintry/catchblocks. If an error is thrown in this method, then we attempt to
recover with
Doc._hardRollback, which will attempt to reset thedocument and then call the callbacks of the pending ops with the error.
This change also prevents
Doc._hardRollbackfrom emitting an error ifat least one pending op callback was invoked, so that we don't branch
error handling behaviour.