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

Stale closure bug in useDecision #170

Closed
fabb opened this issue Jun 15, 2022 · 9 comments
Closed

Stale closure bug in useDecision #170

fabb opened this issue Jun 15, 2022 · 9 comments

Comments

@fabb
Copy link

fabb commented Jun 15, 2022

I'm running into a stale closure bug in my application. My component with the useDecision hook rerenders multiple times (on purpose) and during one of the last rerenders, the overrides (3rd parameter) to the useDecision hook change (on purpose). I have an experiment that targets this exact user attribute. When setting breakpoints, I can see that the experiment is activated, and then directly deactivated again (I have autoUpdate active which I need due to the changing user attributes).

I found that the last deactivation is caused by the 1st useEffect callback in the useDecision hook function. It calls an old stale version of getCurrentDecision which uses old stale overrideAttrs which are not up-to-date and are missing the overrides I provided in the last call to useDecision.

You have a stale closure problem because you are violating the rules of hooks. The function getCurrentDecision is not part of the dependency array of the useEffect. You need to find a way to solve this, probably using a useRef to use an up-to-date version of that function. (unfortunately useEvent has not yet been released, but maybe you could use a polyfill).

@fabb
Copy link
Author

fabb commented Sep 15, 2022

@opti-jnguyen can you reproduce the issue and do you know how to solve it in the optimizely react-sdk?

my problem is that often the experiment is not activated because of this bug, but it's flaky because of the race condition.

@Tamara-Barum
Copy link

Internal Ticket: FSSDK-8665

@rafinutshaw-optimizely
Copy link
Contributor

Hi @fabb, Thanks for raising this issue. It will be very helpful if you could provide an example scenario where this exact problem occurs so that we can look into it thoroughly.

Thanks!

@russell-loube-optimizely

Hi @fabb , apologies for the very prolonged delays with initial feedback on this. Are you able to provide some example scenarios (e.g., code snippets) where this occurs? We're unfortunately unable to continue analyzing without some more information. We hope to hear from you soon.

@fabb
Copy link
Author

fabb commented Jul 27, 2023

Ooff, that was a year ago, I need to check if I can find the scenario where I used this. But it should be reproducible just with the description in my initial comment, you just need multiple rerenders in fast succession with different overrides.

@mikechu-optimizely
Copy link
Contributor

mikechu-optimizely commented Dec 8, 2023

I worked through using useDecision with (randomly) overriding the userId, but was unable to reproduce a race condition, at least in my contrived React app setup. I also cross-referenced our tickets to find where other clients may have reported stale decision.

Thanks for linking and to the point in code where the rule of hooks was violated.

Perhaps you could give a PR to outline your solution?
Or do you have a repo that you can link to me showing the reproducible scenario instead of my lame setup?
Did you work around the problem and can provide guidance there?

@mikechu-optimizely
Copy link
Contributor

Oop, found another work ticket (FSSDK-9624) and more importantly, related Issue #196 and PR #184. That's helpful.

@fabb
Copy link
Author

fabb commented Dec 9, 2023

Hi,
back then we restructured our app and pulled up the useDecision to a component where not so many rerenders happened, avoiding the bug.
Sorry, I cannot put in more time to create reproductions. I don’t mean to be rude, but our company pays a lot for the optimizely license, and I‘m expected to deliver features for our company, not help your company to improve your products.

@mikechu-optimizely
Copy link
Contributor

Thanks for providing the history and your honest feedback.

I'll work this issue via the other GH reports, closing this issue to reduce noise.

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

5 participants