Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unclear error when trying to use instance after .destroy #4339

Closed
gr2m opened this issue Sep 15, 2015 · 11 comments
Closed

Unclear error when trying to use instance after .destroy #4339

gr2m opened this issue Sep 15, 2015 · 11 comments
Assignees
Labels
enhancement Feature request

Comments

@gr2m
Copy link
Contributor

gr2m commented Sep 15, 2015

We have a failing test in https://github.com/hoodiehq/pouchdb-hoodie-api after upgrading to 4.0.2: hoodiehq/pouchdb-hoodie-api#78

I could track down the issue to this

// run with node.js
var PouchDB = require('pouchdb')
var db = new PouchDB('test', {
  db: require('memdown')
})

// just to make sure the init of the db is finished properly
// this can be left out, it has no effect in that context
db.info()

.then(function () {
  return db.put({
    _id: 'cleanTest'
  })
})

  .then(function () {
    console.log('cleanTest created')
    return db.destroy()
  })

  .then(function () {
    console.log('db removed')

    // PROBLEM:
    // this one does never settles settled.
    return db.get('cleanTest')
  })

  .then(function () {
    console.log('.then called after db.get("cleanTest")')
  })
  .catch(function () {
    console.log('.catch called after db.get("cleanTest")')
  })

It's very odd. The node script ends without either calling neither .then or .catch at the end. How is that even possible?

@gr2m gr2m changed the title [4.0.2 ] db.get not settled after calling db.destory when run in Node.js [4.0.2] db.get not settled after calling db.destory when run in Node.js Sep 15, 2015
@NickColley
Copy link
Contributor

Confirmed with @gr2m that any api methods fail, for instance a .put

Also this is only an issue with leveldown and *down based adapters.

@NickColley
Copy link
Contributor

stores.docStore.get(id, function (err, metadata) {

Doesn't return at all after db.destroy

@NickColley
Copy link
Contributor

  1. check this out (#4339) - Failing get after destroy test #4340
  2. run GREP=test.basics.js npm test
  3. The get never returns

@nolanlawson
Copy link
Member

Awesome work tracking this down! So I definitely think this is more of a documentation issue than a bug in PouchDB. My opinion about the destroy() API is that it's the event horizon - once you call it, you never hear from that database object again. The solution is:

var db = new PouchDB('foo');
return db.destroy().then(function () {
  db = new PouchDB('foo');
  return db.get('bar');
});

That said, I do see a bug if the promise never even rejects. We should definitely reject if you try to do anything on a destroyed database.

@daleharvey
Copy link
Member

Oops, commented on the PR but probably more relevant here, tl;dr is as nolan said

Yup this has come up before, the behaviour this is testing was never one that we have catered for, it would have sometimes worked (and sometimes not) n the http adapter, I think almost always break in any of the other adapters, when you call db.destroy we generally expect that to be the end of the life of the object, if you need another db handle you need to reopen it.

We can wipe out the handle when destroy is called, make it throw an obvious error if someone calls any further functions on it, we could also support this use case (being able to reuse the handle after destroy by making all the constructor stateless.

As it stands this isnt really a bug and more an unfriendly error message

@daleharvey daleharvey added the enhancement Feature request label Sep 16, 2015
@daleharvey daleharvey changed the title [4.0.2] db.get not settled after calling db.destory when run in Node.js Unclear error when trying to use instance after .destroy Oct 6, 2015
@alxndrsn
Copy link
Member

alxndrsn commented Jan 8, 2016

I'm interested in helping improve the error messages. Presumably you would want tests written for this; would test.basics.js be a suitable location?

@daleharvey
Copy link
Member

Yup test.basics would be totally fine for this, thanks for picking this up. Feel free to ask here or in irc / wherever if you need any help getting it implemented

@medihack
Copy link

Glad to find this issue here as I also ran into this problem when trying to clean up the db by using destroy in some beforeEach Mocha.js hook. It simply hangs afterwards when trying to work with the same db (without reassignment). This should indeed be better documented, or better solved as destroy is very nice to have when testing. Also in some circumstances a new assignment make things more difficult.
If that can't be implemented easily, how about a .clear method (that takes a bit longer) that also wipes the db?

@nolanlawson
Copy link
Member

@medihack At this point I think we'd prefer just to have an error thrown, instead of hanging. :) No one's had time to look at this recently, but if anyone wants to contribute the fix, they'd be welcome to!

@daleharvey
Copy link
Member

Fixed in 9549fda

@tiagocpontesp
Copy link

This seems so odd! Not a big deal but IMHO it warrants a mention in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request
Projects
None yet
Development

No branches or pull requests

7 participants