Make cached read asynchronous

[clehner]

Make read always asynchronous, even if cached, to avoid stack growth in applications that recurse through calling read.

As explained in #10

[clehner]

cc @flumedb

[arj03]

I was talking to Andre about this and a better way would be to use looper. See ssbc/async-append-only-log#3

[clehner]

@arj03
looper does not accept or pass any function arguments. We are passing data in the function calls in the code path triggering the crash. Unlike async-flumelog which has a streaming API, here we just have get calls, and I don't see any place where looper could be neatly inserted. The readme of looper points to tail-call as a similar module that preserves function argument. Using tail-call or similar could work.

It sounds like you are concerned about performance. tail-call mentions performance issues; looking into its source I see array operations and calling Function.prototype.apply. So I can't assume that it would necessarily be more performant than using setImmediate. I have noticed any performance impact when using this patch.

I am unable to further test/develop for this issue now, as I don't have any known-crashing flumelog+index states. I am running most of my ssb-servers with this patch applied now so am unlikely to encounter the issue any time soon. Even unpatched, I would only see this issue very infrequently.

I encourage you to consider merging this patch, as it fixes a bug causing a crash that leaves users stranded if they don't know how to open a GitHub issue or re-sync their db. While the crash is infrequent, it has been reported by multiple people (ssbc/patchwork#1320), and I think we can assume there are more unreported cases.

[arj03]

What do you think @staltz

[staltz]

I think fixing the crash is a priority, then we can figure out a more performant way of doing it in a separate PR.

It may turn out that our solution to performance is just migrating to jitdb etc.

[arj03]

I ran bench.js with and without the patch, seems the hit is pretty significant:

arj@arj:~/dev/flumelog-offset$ node bench.js 
name, ops/second, mb/second, ops, total-mb, seconds
append, 292672.095, 43.735, 2929355, 437.744, 10.009
stream, 295715.223, 44.189, 2929355, 437.744, 9.906
stream no cache, 329474.187, 49.234, 2929355, 437.744, 8.891
stream10, 453602.339, 67.783, 4536477, 677.905, 10.001
random, 29839.916, 4.459, 298429, 44.595, 10.001

arj@arj:~/dev/flumelog-offset$ node bench.js 
name, ops/second, mb/second, ops, total-mb, seconds
append, 285122.687, 42.604, 2851512, 426.087, 10.001
stream, 99922.407, 14.93, 999324, 149.323, 10.001
stream no cache, 108209.379, 16.169, 1082202, 161.707, 10.001
stream10, 214070.592, 31.987, 2140920, 319.911, 10.001
random, 62858.614, 9.392, 628649, 93.935, 10.001

[staltz]

So approx. 50% slower.

Now depending on who you ask, 50% slower might be considered as bad as an occasional crash. Or not. Tricky to arbitrate.

[staltz]

Thinking a bit more about this, I would probably do this:

• Merge and make a new release (minor or major)
• People who want the faster one can use the previous version
• (I can) write a test that reproduces this error (on the previous version)
• (I can) write a solution that uses looper or something similar to looper and send a PR here