fix: ensure initCache setter function stays within bounds of subscriptions

[lfbergee]

This PR closes #2357, by creating a copy of the subscriptions[key] array in the setter function for initCache. By doing this we avoid an intermittent bug where subscriptions[key] is mutated while the for-loop is running, potentially causing an out of bounce lookup.

By copying the object, mutations of the original object will not affect the setter function while executing. Alternatively we could lock write operation while running the read operation. Or use a different iterator method, that make it's own copy instead of a vanilla for-loop.

This is somewhat hard bug to reproduce, as it's highly intermittent.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 296a994:

Sandbox	Source
SWR-Basic	Configuration
SWR-States	Configuration
SWR-Infinite	Configuration
SWR-SSR	Configuration

[huozhi]

Thanks for the PR! Can you add a test for this?

[lfbergee]

Thanks for the PR! Can you add a test for this?

It's quite challenging making a test that reproduce the original problem, as it looks to me as a race condition when two or more caches are initiated at the same time with the same key for the global store, but different number of subscriptions. I can add a unit test for the initCache, but haven't found a way reproduce the error in isolation. I've tested the fix in my application code where I do have the error intermittently, also shipped a patched version to PROD, and this do eliminate the error, no more reporting in Sentry either.

[promer94]

This problem may invovle some implementaion details of react which makes it hard to create a test case.

When sub[i] is called, React will be told to update. If the update happened immediately and cause some swr hooks umounted, then sub will be changed and it is not safe to call sub[i] anymore

However when i try to reproduce this problem, i noticed that the update won't happen right way. It seems that all the updates will be batched.

I think we could merge this now and add test case later.

[promer94]

Suggested change

	const subs = subscriptions[key].slice()
	const subs = subscriptions[key]
	if (subs) {
	for (const fn of subs) {
	fn(value, prev)
	}
	}

Maybe we could use for of here, so it don't need a extra copy of subs here ?

[lfbergee]

Yeah, that should probably achieve the same. I can update on Monday as soon as I have access to a computer! Thanks for feedback and attention!