holepunchto / hypercore

Hypercore is a secure, distributed append-only log.
https://docs.holepunch.to
MIT License
2.59k stars 182 forks source link

cb never gets called if invoking download with no arguments. #289

Closed RangerMauve closed 2 years ago

RangerMauve commented 3 years ago

When you invoke core.download(cb) without specifying a start and end, it never invokes your cb.

Here's a rough example with hyper-sdk:

const SDK = require('hyper-sdk')

run()

async function run () {
  const { Hypercore: H1, close: close1 } = await SDK({ persist: false })
  const { Hypercore: H2, close: close2 } = await SDK({ persist: false })

  try {
    const c1 = H1('example', {
      valueEncoding: 'json'
    })

    await c1.append('Hello World!')

    const url = `hyper://${c1.key.toString('hex')}`

    const c2 = H2(url, {
      valueEncoding: 'json'
    })

    await c2.ready()

    await new Promise((resolve) => c2.once('peer-open', resolve))

    await c2.update({ifAvailable: true})

    await c2.download()

    // This reaches, though
    // await c2.download({start: 0, end: c2.length})

    console.log('Should reach here but doesnt')

  } finally {
    await close1()
    await close2()
  }
}

I think this is due to end getting set to -1 here which means it'll keep downloading new bits of data and never be finished.

I propose changing the default range to this.length so that the download() completes after the current data has been downloaded. I think this would be a breaking change.

An alternative might be to document that the cb won't get invoked if you don't specify an end so that folks don't get tripped up by it (spent 20 mins adding debug statements all over the place wondering why I was stuck 😂 ).

mafintosh commented 3 years ago

Ya, I'm not happy with the current api here either. I think we should add a live flag instead of the -1 thing to make it super clear (defaults to false). wdyt about something like that?

RangerMauve commented 3 years ago

I think a live flag would be great. Defaulting it to false sounds like it'd be more intuitive, too. I kinda got what -1 meant from looking at the sparse flag in the constructor, but it was a lucky guess. 😅

mafintosh commented 2 years ago

changed in 10