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

empty data buffer on JavaScript cursor when it is closed #5432

Closed
larkost opened this issue Feb 20, 2016 · 5 comments
Closed

empty data buffer on JavaScript cursor when it is closed #5432

larkost opened this issue Feb 20, 2016 · 5 comments
Assignees
Milestone

Comments

@larkost
Copy link
Collaborator

larkost commented Feb 20, 2016

If you call close on a Python or Ruby cursor object, and then try to read from it again you will get an error. However, on JavaScript it will continue to give you data out of the buffer until it is empty (and then error). We should probably do what we do on the Python one, and empty out the buffer immediately.

Note, this is related to #5056.

@danielmewes danielmewes added this to the subsequent milestone Feb 20, 2016
@danielmewes danielmewes modified the milestones: 2.3, subsequent Mar 21, 2016
@danielmewes danielmewes self-assigned this Mar 21, 2016
@danielmewes
Copy link
Member

I have an implementation of this in daniel_5432.

@danielmewes
Copy link
Member

@nighelles You merged this with your eachAsync changes right?

@hueniverse
Copy link

This should have probably been documented as a breaking change. It is described as a cleanup fix but it actually adds a new error which existing v2.2.0 is probably not expecting.

@danielmewes
Copy link
Member

It looks like the new version throws a ReqlDriverError with the message "Cursor is closed.", while the old version might have thrown a different error, but in general had undefined behavior (it would sometimes return more rows and never throw an error, sometimes return a few results and then throw an error, and sometimes throw an error immediately).

The new behavior for the first time defines what the behavior for reading from a cursor after closing it is, so I'm not sure if this qualifies as a breaking change.

In general we should get better at pointing out changed error messages in our release notes though. We are currently not mentioning such changes anywhere.

@hueniverse
Copy link

The exception was new in the way I was using it and it broke a bunch of tests. The release was already a breaking release so there was no issues there, just that this change was billed as a minor fix when it actually changed significant behavior. It was the right change to make.

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

No branches or pull requests

3 participants