Cancelled watches

[aikoven]

In rare cases watch promises resolve to false after a transaction is committed, even though I don't call cancel method.

In future.cpp I see this code:

        err == 1101 // operation_cancelled
        || err != 1025 // transaction_cancelled
        || err != 1020)) { // not_committed (tn conflict)

Shouldn't it be == in all three cases? Or am I misunderstanding something?

[josephg]

Yeah that does look wrong - actually, how can it ever return false with that condition?

Good spotting. I’ll take a look

[josephg]

Yep totally a bug. I've fixed it; although with node's default promise rules if a watch fails for other reasons it will crash nodejs.

I'm not sure what the right thing to do here is - I'm tempted to add an empty watch.promise.catch(() => {}) to remove the default behaviour of crashing the process if a watch fails due to a failing transaction.

But with respect to the behaviour you're seeing (the watch resolving to false), I remember reading something in the docs where watches could sometimes resolve even though the watched value hasn't changed. For example, if the network connection is silently reestablished between your client code and the FDB server, the watch will resolve with false, and you're expected to re-establish it. But I can't find the documentation now, so I might be misremembering.

[aikoven]

I think it's fine for a watch promise to resolve to false if the transaction fails for any reason. After the transaction succeeds a user is at least able to catch the rejection. However, in my gist from #28 (comment) I do read + watch in a transaction, then yield results and then await the watch promise and indeed it would crash if the watch fails before we get back from yield. To fix this I would do the same — add an empty catch. Not sure though if that should be done implicitly or it's a user's responsibility.

In any case, I'd like to see the exact reason for watch failure apart from cancellation, to be able to properly handle and report it.

[josephg]

I would have expected that too - but surprisingly the test case I added for this issue works correctly. Its only waiting on the promise after the transaction has returned (later).

Does node only bump rejections to the global exception handler when a failed promise is garbage collected or something?

[aikoven]

I'm not sure about this, afaik NodeJS 10 only prints a warning in case of unhandled rejection.

In our apps, we always have a custom unhandledRejection handler that "gracefully crashes" the app.

NodeJS also has rejectionHandled event that allows for rejections to be handled at a later time. Maybe there's some sort of a timeout by default.

[josephg]

Hm maybe. This is fine:

;(async () => {
  const p = Promise.reject(Error('ermagherd'))
  p.catch(() => {}) //ok
})()

This hits the global unhandledRejection handler:

;(async () => {
  const p = Promise.reject(Error('ermagherd'))
  await new Promise(resolve => setTimeout(resolve, 0))
  p.catch(() => {}) // unhandled rejection handler hits before this
})()

But this is totally fine:

;(async () => {
  const p = Promise.reject(Error('ermagherd'))
  await new Promise(resolve => process.nextTick(resolve))
  p.catch(() => {}) // ok
})()

[josephg]

I've had a think about it and I don't like the default behaviour here. I'm going to give it an empty catch handler to prevent the global unhandledRejection hook from being hit when a watch errors. (The actual watch promise will behave the same way; it just won't crash your program when it happens.) Usually when a watch has a weird error, the error will effect the transaction that created it too. This way you won't have to handle both errors.

[josephg]

📦 foundationdb @ 0.9.1