Ensure that if the CachedAsyncIterable is called multiple times in parallel, it does return the correct value #4
Conversation
…rallel, it does return the correct value
|
I had to also modify I'm afraid that this makes the "sync iterator over cached items in async iterator" scenario not work as shown by the two failing tests. |
There was a problem hiding this comment.
We'll need sync iteration for projectfluent/fluent.js#100. I think there might be a way to keep it if we maintain two caches: one for the resolved {value, done} objects and one for the promises returned by iterator.next().
Unless I'm mistaken, this would also make it possible to remove the explicit [Symbol.iterator] method from CachedAsyncIterable. We can just use the default implementation from the Array, which CachedIterables extend.
src/cached_async_iterable.mjs
Outdated
| } | ||
| return cached[cur++]; | ||
| return await cached[cur++]; |
There was a problem hiding this comment.
Can you create a new array for caching the promises returned by iterator.next() rather than pushing directly to this? This way, you should be able to do:
let nxt = await cachedPromises[cur++];
this.push(nxt);
return nxt;
|
@zbraniecki Any updates on this? Should we try to publish a new version of |
|
Sorry @stasm - I know it's important, and it also is blocking bmo bug 1483038, but I haven't had the time to properly consume your idea on how to preserve the sync in async, and in result couldn't update the patch. :( Any chance you know how you'd like an update to this patch to look like and can provide a patch to my PR? |
I know what I want it to look like but I might not have time to do it this week nor the next week. Let me see if I can put together a WIP tomorrow morning. To reiterate, I think we should keep a separate cache for promises returned from |
I have to admit that I struggle to see the value in this. It seems to me like an additional allocation either via strong or weak ref to store data that the async iterator doesn't need. My confusion may come from the fact that I don't understand the use case of a sync iterator over async iterator. I'll wait for your WIP - maybe it'll enlighten me :) |
I explained the use-case in #1. |
|
I haven't had the time to look into this this week and I doubt my next week looks any better. If this fix is important, let's remove the sync iterator for now. projectfluent/fluent.js#100 is still weeks from landing. I'll be happy to review an updated PR. |
|
Removed sync iterator over async cachediterable and marked the tests to be skipped for now. |
There was a problem hiding this comment.
Thanks. Please remove the skipped tests, there's no need to keep them right now. We can always resurrect them from git history once we need them. Please also add a short description of this change to CHANGELOG. With that, r=me, feel free to land it yourself. Thanks again!
test/cached_async_iterable_test.js
Outdated
| const iterable = new CachedAsyncIterable(generate()); | ||
| await Promise.all([ | ||
| iterable[Symbol.asyncIterator]().next(), | ||
| await iterable[Symbol.asyncIterator]().next(), |
There was a problem hiding this comment.
Please add another test in which you call next twice on the same asyncIterator.
There was a problem hiding this comment.
Oh, and please assert in all tests that these are actually the same promises.
test/cached_async_iterable_test.js
Outdated
|
|
||
| // We're testing that if the first call to asyncIterator has been | ||
| // made, but the value of it has not been returned yet, | ||
| // the consequitive call returns the same Promise rather than, |
test/cached_async_iterable_test.js
Outdated
| async function *generate() { | ||
| while (true) { | ||
| counter++; | ||
| yield await Promise.resolve(); |
There was a problem hiding this comment.
I think just yield null; is enough here.
| const iterable = new CachedAsyncIterable(generate()); | ||
| await Promise.all([ | ||
| iterable.touchNext(2), | ||
| iterable[Symbol.asyncIterator]().next(), |
There was a problem hiding this comment.
Maybe add another test where touchNext is actually called multiple times :)
Nasty little race condition here that was actually messing with Firefox in production already :(