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

Iterators are not always released #152

Closed
penberg opened this issue Nov 1, 2021 · 4 comments
Closed

Iterators are not always released #152

penberg opened this issue Nov 1, 2021 · 4 comments

Comments

@penberg
Copy link
Contributor

penberg commented Nov 1, 2021

The Chisel.find_all() JavaScript function returns an iterator that is backed by a Rust QueryStreamResource object, which needs to be released at some point.

In JavaScript, you can implement a "return()" function for an iterator, which is called if the iterator is partially consumed.

For example, if you do the following (break out of iteration loop after first element):

let people = await Chisel.find_all("Person");
for await (let person of people) { 
  response += person.first_name;
  response += " ";
  break;
} 

The "return()" function is called to clean up. Our Chisel.find_all() iterator implements release() so this part is fine.

However, if you do:

let people = await Chisel.find_all("Person");

or even:

let people = await Chisel.find_all("Person");
people.next();

The "return()" function is not called, leaving the iterator alive. For us, this means that user can potentially leave QueryStream objects, for example, alive...

@espindola
Copy link
Contributor

We should implement return(), and that is probably OK for version 0.1 or 0.2. The last line of defense is to use the GC, which can be used with:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry

A good example is at https://github.com/denoland/deno/blob/main/ext/fetch/26_fetch.js#L95

@penberg
Copy link
Contributor Author

penberg commented Nov 1, 2021

@espindola We do implement return() already: https://github.com/chiselstrike/chiselstrike/blob/main/server/src/chisel.js#L39

The issue is about the corner cases it does not cover.

@espindola
Copy link
Contributor

@espindola We do implement return() already: https://github.com/chiselstrike/chiselstrike/blob/main/server/src/chisel.js#L39

Nice, I missed that somehow. We should add a test to show that it is working :-)

@penberg
Copy link
Contributor Author

penberg commented Aug 5, 2022

We now explicitly iterate over all resources and close them in closeResources() after endpoint invocation so closing this issue.

@penberg penberg closed this as completed Aug 5, 2022
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

No branches or pull requests

2 participants