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

[REF-2219] Computed and cached vars based on rx.Cookie and rx.LocalStorage not getting updated starting from 0.4.3 #2851

Closed
PasqualePuzio opened this issue Mar 13, 2024 · 9 comments · Fixed by #2886 or #2953
Labels
bug Something isn't working linear Created by Linear-GitHub Sync

Comments

@PasqualePuzio
Copy link

PasqualePuzio commented Mar 13, 2024

Describe the bug
In our project we have a few computed (some of which are cached) vars which are based (they perform some calculations on) on other state vars of type rx.Cookie and rx.LocalStorage.
All of them belong to the same state.
We have noticed that starting from 0.4.3 (until 0.4.2 everything is fine according to our tests) the value of these vars are not consistent with the value of the cookie or the local storage entry. It looks like they're not updated when they should.

To Reproduce

class MyState(rx.State):

  auth_data: str = rx.Cookie('',name='auth_user',max_age=2592000)

  @rx.cached_var
  def auth_user(self) -> dict:
      if self.auth_data is None or self.auth_data == '':
          return None
      try:
          user = json.loads(self.decrypt(self.auth_data))
          return user
      except:
          return None

  @rx.cached_var
  def is_user_logged_in(self) -> bool:
      return self.auth_user is not None and len(self.auth_user) > 0

Expected behavior
We would expect the same behavior we had until 0.4.2, meaning that the values of these vars reflect the value of the underlying rx.Cookie or rx.LocalStorage vars and get updated upon hydration and whenever the cookie value changes.

Specifics

  • Python Version: 3.9.6
  • Reflex Version: 0.4.3 and 0.4.4
  • OS: Mac OS 13.3.1
  • Browser: Chrome 122.0.6261.112

REF-2219

@picklelo picklelo added bug Something isn't working linear Created by Linear-GitHub Sync labels Mar 13, 2024
@picklelo picklelo changed the title Computed and cached vars based on rx.Cookie and rx.LocalStorage not getting updated starting from 0.4.3 [REF-2219] Computed and cached vars based on rx.Cookie and rx.LocalStorage not getting updated starting from 0.4.3 Mar 13, 2024
@masenf
Copy link
Collaborator

masenf commented Mar 19, 2024

@PasqualePuzio How is the cookie / localstorage being set in your app? I'm not able to reproduce this when setting a cookie and navigating to another page.

@masenf
Copy link
Collaborator

masenf commented Mar 19, 2024

Or is the problem when the cookie expires it still appears to be set in the state?

@PasqualePuzio
Copy link
Author

PasqualePuzio commented Mar 19, 2024

@PasqualePuzio How is the cookie / localstorage being set in your app? I'm not able to reproduce this when setting a cookie and navigating to another page.

We call self.set_auth_data(encrypted) on the backend to set the cookie.

Everything works perfectly until version 0.4.2, the issue shows up only starting from version 0.4.3.

@masenf
Copy link
Collaborator

masenf commented Mar 20, 2024

Are you using redis? Do you see this discrepancy in dev mode?

@PasqualePuzio
Copy link
Author

Are you using redis? Do you see this discrepancy in dev mode?

Yes we're using REDIS and we haven't seen any difference between dev and prod.
For your information, we use REDIS both in dev and prod mode to avoid any unexpected surprises when going in prod 🙃 so this issue may only appear when using REDIS but I'm not sure.

@masenf
Copy link
Collaborator

masenf commented Mar 20, 2024

Okay glad to hear you're using redis because I believe the fix I posted a few hours ago addresses this issue.

If you have time to run on that branch to prove the theory that would be amazing.

pip install reflex@git+https://github.com/reflex-dev/reflex@masenf/save-cached-vars

I couldn't quite get to a simple repro example, but a larger app where I'm using cookies and cached_var was able to reproduce it for me to discover the problem.

@PasqualePuzio
Copy link
Author

@masenf I'm sorry but I have to reopen this issue :)

We have upgraded to the latest version (0.4.5) which includes this fix if I'm not mistaken, however the issue still occurs.
We have tested the app without REDIS and indeed if we keep the state in memory everything works fine, so the issue seems to occur only when using REDIS and local storage/cookies.

Let me give you some more details on the structure of our app, maybe it'll help narrow down this issue.

We have a base state called AppState which declares the both the cookie/local storage vars and the cached vars. Something like this:

class AppState(rx.State):

    auth_data: str = rx.Cookie('',name='fm_auth_user',max_age=2592000)
    
    # store encrypted user data into the cookie
    def store_auth_data(self, user: dict) -> None:
        self.set_auth_data(self.encrypt(json.dumps(user)))

    @rx.cached_var
    def auth_user(self) -> dict:
        if self.auth_data is None or self.auth_data == '':
            return None
        try:
            user = json.loads(self.decrypt(self.auth_data))
            return user
        except:
            return None
    
    @rx.cached_var
    def is_user_logged_in(self) -> bool:
        return self.auth_user is not None and len(self.auth_user) > 0

then we have a number of children states which inherit from the parent state above:

class MyPageState(AppState):
  
  ...

Here's what we have observed:

  • login works fine, if we stay in the same browser tab and navigate via internal links, we're able to navigate all the pages of the app without any problem
  • however, if we open a new tab or type the url of a valid page in the browser (without navigating via internal links), the value of is_user_logged_in is not up-to-date, we still see the user as not logged in

based on the observations above, I suspect it has to do with hydration or state management (maybe some vars are not saved/loaded correctly?).

Please let me know if there's anything else we can do to help narrow this down.

PS in this latest version the flickering effect I was mentioning here https://github.com/orgs/reflex-dev/discussions/2885 is way less noticeable, it lasts just a fraction of a second instead of 1-2 seconds like it was until in version 0.4.2

@masenf masenf reopened this Mar 21, 2024
@masenf
Copy link
Collaborator

masenf commented Mar 28, 2024

I think we have narrowed this one down to #2950, trying to work out an actual minimal repro/smoking gun so the fix can be verified.

@masenf
Copy link
Collaborator

masenf commented Mar 28, 2024

Yes! Minimal repro achieved!

import reflex as rx


class State(rx.State):
    cookie: str = rx.Cookie()

    @rx.var
    def v(self):
        pass


class Child(State):
    cookie2: str = rx.Cookie()


def index():
    return rx.button(
        f"Set cookie: '{State.cookie}' '{Child.cookie2}'",
        on_click=[State.set_cookie("foo"), Child.set_cookie2("bar")],
    )


app = rx.App()
app.add_page(index)

Load the page. Click the "Set cookie" button. Notice that the button text changes to include 'foo' 'bar'. Refresh the page. Now the button text contains '' 'bar'. The cookie from the parent State is not present, even though the cookie exists in the browser.

Screen.Recording.2024-03-28.at.12.46.41.mov

masenf added a commit that referenced this issue Mar 28, 2024
The already cached states may have unsaved changes which can be wiped out if
they are refetched from redis in the middle of handling an event.

If the root state already knows about one of the potentially missing states,
then use the instance that is already cached.

Fix #2851
masenf added a commit that referenced this issue Mar 29, 2024
* Add test_get_state_from_sibling_not_cached

A better unit test to catch issues with refetching parent states
and calculating the wrong parent state names to fetch.

* _determine_missing_parent_states: correctly generate state names

Prepend only the previous state name to the current relative_parent_state_name
instead of joining all of the previous state names together.

* [REF-2219] Avoid refetching states that are already cached

The already cached states may have unsaved changes which can be wiped out if
they are refetched from redis in the middle of handling an event.

If the root state already knows about one of the potentially missing states,
then use the instance that is already cached.

Fix #2851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working linear Created by Linear-GitHub Sync
Projects
None yet
3 participants