search

projectfluent / cached-iterable

Iterables which cache the values they yield
6 stars 2 forks source link

Ensure that if the CachedAsyncIterable is called multiple times in parallel, it does return the correct value #4

Closed zbraniecki closed 6 years ago

zbraniecki commented 6 years ago

Nasty little race condition here that was actually messing with Firefox in production already :(

zbraniecki commented 6 years ago

I had to also modify touchNext to avoid a scenario (experienced in production) where touchNext is still operating and code calls for await (let ctx of this.ctxs) and we start fetching things again because this.seen is still waiting for touchNext to complete.

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.

stasm commented 6 years ago

@zbraniecki Any updates on this? Should we try to publish a new version of cached-iterable with this fix soon?

zbraniecki commented 6 years ago

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?

stasm commented 6 years ago

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 next() and another one for the {value,done} objects these promises resolve to. We'll need to keep track of which promises have already resolved so that we don't add their resolved values to the sync cache more than once. Perhaps changing CachedIterable to extend Set rather than Array would help?

zbraniecki commented 6 years ago

To reiterate, I think we should keep a separate cache for promises returned from next() and another one for the {value,done} objects these promises resolve to.

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 :)

stasm commented 6 years ago

My confusion may come from the fact that I don't understand the use case of a sync iterator over async iterator.

I explained the use-case in #1.

stasm commented 6 years ago

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. https://github.com/projectfluent/fluent.js/issues/100 is still weeks from landing. I'll be happy to review an updated PR.

zbraniecki commented 6 years ago

Removed sync iterator over async cachediterable and marked the tests to be skipped for now.