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

upgrade leveldown and fix iterator problems #6639

Merged
merged 2 commits into from
Jul 23, 2017
Merged

upgrade leveldown and fix iterator problems #6639

merged 2 commits into from
Jul 23, 2017

Conversation

ralphtheninja
Copy link
Contributor

@ralphtheninja ralphtheninja commented Jul 23, 2017

Closes #6551

I was a bit unsure on where to check for the error. We have different options here.

At first I wanted to put an error handler on changeStream here, which could check for that particular error. But unclear to me how a different error should be handled. changeStream is also completely internal to the api._changes function so a bit clunky to handle the errors.

So I tried to go deeper instead and try to stop this error as early as possible and as close to iterator.next(cb) as possible and basically don't even emit it on the stream to begin with.

Let me know if you aren't happy with this solution and/or if it should be tweaked.

Cheers

@ralphtheninja
Copy link
Contributor Author

ralphtheninja commented Jul 23, 2017

I have no idea about some of the errors. Might some of them be related to other things?

Apparently tests did work. Guessing someone restarted them :)

Copy link
Member

@daleharvey daleharvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring this based on the string of an error message is quite fragile, it would be nice to be a little more robust and only ignore errors when we know for example the iterator died after we closed the db, or preferably to close the iterator ourselves.

However very happy to have this unblocked and us back to receiving leveldown updates, its a large complex dep and we dont want to get stuck too far back, happy to merge this and we can look at being little more robust / cleaner as we go on.

Thanks so much at looking into this and dealing with the complexities of the pouch test suite, I know its pretty hairy :)

@daleharvey
Copy link
Member

And yup, the failing tests werent level related and can be a bit flaky, trying to get them more stable but occasionally need a restart

@daleharvey daleharvey merged commit 90c9518 into pouchdb:master Jul 23, 2017
@ralphtheninja
Copy link
Contributor Author

ralphtheninja commented Jul 23, 2017

Ignoring this based on the string of an error message is quite fragile, it would be nice to be a little more robust and only ignore errors when we know for example the iterator died after we closed the db, or preferably to close the iterator ourselves.

@daleharvey Aye, I was thinking exactly the same. I was thinking we could clean that up later by e.g. named errors in e.g. level/errors that abstracts it better. But currently only levelup uses the error module and not leveldown so that has to be changed.

@ralphtheninja
Copy link
Contributor Author

@daleharvey What do you reckon the new version is going to be? A patch?

@ralphtheninja
Copy link
Contributor Author

@ralphtheninja
Copy link
Contributor Author

No wait, that version is not correct either haha. Ok, I think I got it now. Next version will be 6.3.5.

@holsted
Copy link

holsted commented Oct 23, 2017

Any update on when this might land in the next version @daleharvey?

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 this pull request may close these issues.

Update and test with the latest leveldown
3 participants