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

Error: database is destroyed #612

Closed
gr2m opened this issue Oct 21, 2016 · 36 comments · Fixed by hoodiehq/hoodie-store-client#136
Closed

Error: database is destroyed #612

gr2m opened this issue Oct 21, 2016 · 36 comments · Fixed by hoodiehq/hoodie-store-client#136

Comments

@gr2m
Copy link
Member

gr2m commented Oct 21, 2016

We have a problem with sign in / sign out / sign in again and the scoped store API. Here is a script to reproduce the bug

hoodie.account.on('signin', function () {
  // logs error here
  hoodie.store('conversation').findAll().then(console.log, console.log)
})

var username = Math.random().toString(36).substr(2, 5)
hoodie.account.signUp({username: username, password:'test'})

.then(function () {
  return hoodie.account.signIn({username: username, password:'test'})
})

.then(function () {
  return hoodie.account.signOut()
})

.then(function () {
  return hoodie.account.signIn({username: username, password:'test'})
})

.then(function () {
  console.log('done')
})

I looked into it but could not yet figure it out. Thanks to @brunopedroso who found the error

@GreenGeorge
Copy link

GreenGeorge commented Oct 23, 2016

I wanted to open a new issue but I think this is the same issue. Was trying to follow along the hoodie tutorial and I got to the signUp and signIn example. Copy pasted the example code and this is what I got:

hoodie.account.signUp({username: 'Robin', password: 'secret'})

returns

{outcome: { id:"0k4dgh7", username:"Robin"}, ...}

and

hoodie.account.signIn({username: 'Robin', password: 'secret'})

returns

{ outcome: "Invalid credentials" , ...}

and status 401 unauthorized

@gr2m
Copy link
Member Author

gr2m commented Oct 23, 2016

@GreenGeorge that’s a different issue. Could you create a separate issue on this repository to keep the thread here on the original issue?

What tutorial did you follow? Can you run npm ls | grep hoodie and paste the output in the new issue please? That’ll help us debugging

@GreenGeorge
Copy link

@gr2m will do

@brunopedroso
Copy link
Contributor

@gr2m I must say I could not reproduce the error with your code.

I started a new app just like described here, then I put your code on a new script tag in index.html and what I see is a 401 when I try to logout...

client.js:25722 DELETE http://localhost:8080/hoodie/account/api/session 401 (Unauthorized)

Am I doing something wrong?

[email protected] /Users/bruno/workspace/test_hoodie_cli_2
`-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | | +-- [email protected]
  | | +-- [email protected]
  | | `-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]

@gr2m
Copy link
Member Author

gr2m commented Oct 25, 2016

@brunopedroso do you use CouchDB as the backend? Did you use 'Robin' as the username? There is an issue with capitalization hoodiehq/hoodie-account-client#115

@brunopedroso
Copy link
Contributor

No, I tested it without couchdb. Just pouch and fs in a freshly created app.
I also do not use Robin as username... (thats George's example, that looks unrelated). I used your code that randomply generates usernames with
var username = Math.random().toString(36).substr(2, 5)

@brunopedroso
Copy link
Contributor

I upgraded node, npm and hoodie and keep seeing the same 401..

bruno@MacBook-Pro-de-Mac teste $ node -v
v6.8.1
bruno@MacBook-Pro-de-Mac teste $ npm -v
3.10.9
bruno@MacBook-Pro-de-Mac teste $ npm ls | grep hoodie
`-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | | +-- [email protected]
  | | +-- [email protected]
  | | `-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]
bruno@MacBook-Pro-de-Mac teste $ 

@brunopedroso
Copy link
Contributor

I added a catch and could see a

(index):38 er Error: Authorization header missing(…)
(anonymous function) @ (index):38
(anonymous function) @ client.js:5973
nextTick @ client.js:5807

@brunopedroso
Copy link
Contributor

And it seems to be right here.

@brunopedroso
Copy link
Contributor

I can see a request header in chrome console: authorization:Bearer cjNyaWo6NTgyMUI3MjU6UkPSyRFKKYo29p2uo6ADmuMjw3M

@brunopedroso
Copy link
Contributor

Hmmm... Looks like a versions issue..

@brunopedroso
Copy link
Contributor

is this dependecy right?

@brunopedroso
Copy link
Contributor

Actually, I see 2 different versions of account-client here:

bruno@MacBook-Pro-de-Mac teste $ npm ls | grep hoodie
`-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  | | +-- [email protected]
  | | +-- [email protected]
  | | `-- [email protected]
  +-- @hoodie/[email protected]
  | +-- @hoodie/[email protected]
  +-- @hoodie/[email protected]
  | `-- @hoodie/[email protected]

@brunopedroso
Copy link
Contributor

yeah! could finally reproduce the error hacking hoodie-client module and rebuilding it poining to [email protected].

Maybe we should open another issue for this dependency problem?

@brunopedroso
Copy link
Contributor

I could reproduce it without the findAll and the signup:

      var username = 'teste'//Math.random().toString(36).substr(2, 5)
      //hoodie.account.signUp({username: username, password:'test'})
      Promise.resolve()

      .then(function () {
        console.log(1)
        return hoodie.account.signIn({username: username, password:'test'})
      })

      .then(function () {
        console.log(2)
        return hoodie.account.signOut()
      })

      .then(function () {
        console.log(3)
        return hoodie.account.signIn({username: username, password:'test'})
      })

      .then(function () {
        console.log(4)
        console.log('done')
      })

      .catch(function (e) {
        console.error('er', e)
      })

I see the logs 1, 2 and 3. then 'db is destroyed'

@brunopedroso
Copy link
Contributor

and by the stack I can see the error occurs when post:signing tries to reset the database again.

@brunopedroso
Copy link
Contributor

ops.... Looks like the version of reset I have here is also not up-to-date... I'm running this one. =/

@brunopedroso
Copy link
Contributor

hm... looks like its the same problem? =/

@brunopedroso
Copy link
Contributor

Well,

I could hack the module again, updating the dependency and looks like its a bit better...

If I comment out the findAll hook, it justs passes smoothly, without errors. (signUp -> signIn -> signOut -> signIn).

If it but back the findAll hook, it still shows 'db is destroyed' in the console, but now in 'log' level (not 'error') and it does not stop the rest of the script. It goes on and prints 1,2,3,4,done...

@gr2m
Copy link
Member Author

gr2m commented Oct 25, 2016

"@hoodie/store-client": "^1.0.0",

WOAH this is at version 5 how did that not get updated.

Did you try if updating it fixes the problem?

@brunopedroso
Copy link
Contributor

its defined like so in master of hoodie-client, look:

https://github.com/hoodiehq/hoodie-client/blob/master/package.json#L12

@gr2m
Copy link
Member Author

gr2m commented Oct 25, 2016

I’m looking into it thanks

@brunopedroso
Copy link
Contributor

@gr2m I dug it a lot today and guess I found the problem.

the Store() method returns a function that has other functions as properties. That's the approach used in scoped api that allows us to do both

  // here's how I get the scoped api.
  var scoped = hoodie.store('conversation');

  // I dont need to pass types to its members any more
  scoped.findAll();

or

  // store is the 'unscoped' api. I need to pass type for its members
  hoodie.store.findAll('conversation');

This approach seems to be incompatible with the way store.reset() works - it replaces all store's members with other ones that are now binded to a new db.

It works in 'uscoped' api, when we call store's members, because they've been correctly replaced. But it cannot replace the store function itself... We can merge properties to the api function, but we cannot change the function itself. Its been passed 'by value' and the caller now has a reference to it. You cannot replace its reference anymore... So we end up with scoped api thats still bound to the old db (that is obviously already destroyed!). =(

I tried some hacks to overcome this, but it seems to me it's not the right way...

Maybe we could just use instead:

  hoodie.store.scoped('conversation').findAll();

Its not the best api but it would work properly with reset()...

what do you think? does it make any sense?

@gr2m
Copy link
Member Author

gr2m commented Oct 26, 2016

Thanks for looking into it. The reset method is way overdue for a proper cleanup, it’s quite messy. I’ll take the opportunity to do that over the weekend, I’ll keep you posted what I found out, maybe I find a way to work around all this

@gr2m
Copy link
Member Author

gr2m commented Nov 16, 2016

just a quick update, I tested the code in the latest hoodie with all submodules up-to-date, the issue is not yet fixed, I’m looking into it

@gr2m
Copy link
Member Author

gr2m commented Nov 16, 2016

okay I understand the problem now, took me a while :) This line is what’s not working, we can as well just leave it out here, it won’t make any difference.

I think what I would do is to keep the current PouchDB instance in an internal state and pass the internal state. For that we would have to merge pouchdb-hoodie-api and pouchdb-hoodie-sync into hoodie-client-store which is probably a good idea anyway at this point. It was a good idea at the time but with the scoped API, the separation of pouchdb-hoodie-api and hoodie-store-client is rather troublesome.

The dedicated .scoped() method would not work because for code like this:

var todoStore = store.scoped('todo')

store.reset().then(function () {
  todoStore.findAll()
})

Passing the internal state on which we can set the PouchDB database instance would though.

Any thoughts?

@gr2m
Copy link
Member Author

gr2m commented Dec 4, 2016

lots and lots of updates in Hoodie land, but didn’t yet get to this one. I had some thoughts though on how we cam resolve this problem properly, it’s on my list, I won’t forget! Sorry this takes so long!

@brunopedroso
Copy link
Contributor

No problem. Thanks! I must take some time to review your last comment as well. Sorry. Will try to do this asap.

@brunopedroso
Copy link
Contributor

@gr2m , I read your last message with attention now. Just to check if we're in the same page: when you say 'this line is what's not working', actually the merge works. The properties from scope do get merged in api. But it does not rebind the values the api function already has bound (it was first bound here, and as long as I understand cant be rebound...)

Concerning the solution I proposed, in fact, the code you imagined would not work, because the reset would replace the scoped function with a new one (bound to the new DB), so, the scoped api would need to be redeemed again... something like

store.reset().then(function () {
  store.scoped('todo').findAll()
})

But, for sure, if someone retains a reference to the scoped apis, that wont work agian.

Concerning your suggested solution, I'm not sure if I fully understand what you mean by 'passing and keeping the internal state', so I'm not able to comment. Maybe you could sketch some sample code?

I also cannot say if merging pouchdb-hoodie-api is a good idea. I can only say that, at first, the hoodie project seems so much sliced in subprojects. I had some issues understanding the code because of that. But once I dont know the original motivations, I also cant say if its a good idea.

@gr2m
Copy link
Member Author

gr2m commented Dec 26, 2016

@brunopedroso hey Bruno, I’d love to hear your thoughts on this discussion regarding the .type property and the hoodie.store(scope) API: hoodiehq/discussion#106

@fredguth
Copy link
Contributor

fredguth commented Feb 4, 2017

Friends, any news on this one?
Is there a plan of attack on this issue? How can one help solve this?

@pmbanugo
Copy link
Member

pmbanugo commented Feb 4, 2017

I get this error if I have the user logged-in to my app and later logout and then try to add data without being logged-in. If I refresh the app/url before adding new item it doesn't happen

@gr2m
Copy link
Member Author

gr2m commented Mar 4, 2017

news: hoodie.store() was replaced with hoodie.store.withIdPrefix() as discussed in hoodiehq/discussion#106. It’s not yet in hoodie / @hoodie/client, but it landed in @hoodie/store-client v8

@alterx
Copy link

alterx commented Mar 10, 2020

@gr2m @brunopedroso did this fix made it into @hoodie/client ?

I have a prefixed store created like this const myRoutes = hoodie.store.withIdPrefix('myroutes/');

If I log out, log in again and try to use it, I get the database is destroyed error. I'm using the latest version of @hoodie/client (10.2.0)

@gr2m
Copy link
Member Author

gr2m commented Mar 10, 2020

I'm sorry I don't know, it's been so long :( I think I didn't finish the work for it. If you want to give it a look, I can review your pull requests, but not much else I can do right now, given my time constraints

@alterx
Copy link

alterx commented Mar 12, 2020

I see, I'll try to dig into this a bit more since it seems like the structure changed a lot since this PR was created

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

Successfully merging a pull request may close this issue.

6 participants