holepunchto/autobase

`TypeError: this.ready is not a function` thrown from stray `this.ready()` in `LinearizedCore`

lejeunerenard opened this issue · 11 comments

Got the following error message and did some sleuthing but don't know enough about how everything works to fix it:

.../node_modules/autobase/lib/linearize.js:588
    if (!this.lastUpdate) await this.ready()
                                     ^

TypeError: this.ready is not a function
    at LinearizedCore.get (.../node_modules/autobase/lib/linearize.js:588:38)
    at LinearizedCoreSession.get (.../node_modules/autobase/lib/linearize.js:778:34)
    at Batch.getBlock (.../node_modules/hyperbee/index.js:540:35)
    at Batch.getRoot (.../node_modules/hyperbee/index.js:528:24)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async TreeIterator.open (.../node_modules/hyperbee/iterators/diff.js:81:18)
    at async Promise.all (index 1)
    at async DiffIterator.open (.../node_modules/hyperbee/iterators/diff.js:156:5)

My Sleuthing

I took a look at the LinearizedCore class and didn't see a ready function and couldn't find where it would be added dynamically. So I git blamed the offending line to find it was added here:

if (!this.lastUpdate) await this.ready()

and also noticed that there was a ready function at that time:
async ready () {

Finally I found that this ready function was remove in this commit 02a8ed5.

LuKks commented

Sounds like it could be await this.open(this.view, this.clock) now, but I also don't know about Autobase, do you have a reproducible test case?

LuKks commented

Ah I was basing my previous response on your links but it got further updated, so using open is not valid

Yeah. I think this is a bug from vestigial code from refactoring. I tried replacing the this.ready() with a modern version of the ready function:

  async get (seq, opts = {}) {
    if (!this.lastUpdate) {
      await this.autobase.ready()
      const clock = this.clock || await this.autobase.latest()
      await this._rebuild(clock)
    }
    ...
  }

And the error went away and then I got another unrelated error. Still don't know if this is an actual fix though.

I'll see if I can write a test to recreate it when I have a chance.

@LuKks Found a quite simple test to replicate the error:

test('local linearizing - get() awaits for update if not every updated before', async t => {
  const output = new Hypercore(ram)
  const writerA = new Hypercore(ram)

  const base = new Autobase({
    inputs: [writerA],
    localOutput: output,
    autostart: true,
    eagerUpdate: false
  })

  await base.append('a0', await base.latest(), writerA)
  t.doesNotThrow(async () => await base.view.get(0))

  t.end()
})

I tried replacing the await this.ready() my solution above and it does pass everything then. So maybe that's the solution? The check on this.lastUpdate seems to be to ensure its set when calling _get() lest it return null and trigger a rebuild anyways.

I am having the same this.ready not a function problem regarding LinearizedCore.

image

My fix is, make sure it updates before getting. I removed the pesky line, until there is official fix. The fix above doesn't completely fix the problem for me.

@zacharygriffee How does my fix not fix the problem for you? Are you still getting the same error with it?

No I do not get the same error. I get core doesn't find the latest appends without an update in user-land. With an update in user-land then the linearized core will have 'lastUpdate'. Correct me if I'm wrong, if I have to do the update in user-land anyways, that makes the if statement completely useless. So in a way, your fix, in my mind works, it just from the logic I have, it's irrelevant anyways. So poor wording on my end unless I'm not seeing something I should.

edit: I meant linearized core not view

My line of thinking that is why it was missed by the devs anyway, cause docs show to update before view.get, and update makes that if statement irrelevant.

You're correct that if an .update() is required before calling .get() then the if statements irrelevant. But I would say then an error should be thrown when this.lastUpdate isn't set.

However I don't think that .update() should be required. I can imagine a scenario where I want to check the view blocks before ever updating. Maybe even to compare it with what the view looks like after updating. This would of course still effectively call an update without doing session migration. I think though that this even ideally wouldn't happen as the view shouldn't require having been updated to read from it. Before that happens I figure building in the guard (recreating it as it was previously) at least fixes requiring a call to .update().

I agree. I would definitely prefer not having to manually call update.. I probably ended up here thinking I could get the non-updated linearized core if I didn't update before get, and the way I been working with vanilla hypercores as well.