Coflnet / hypixel-react

GNU Affero General Public License v3.0
10 stars 18 forks source link

Coflcoins update weirdly when multiple tabs are open #1234

Closed matthias-luger closed 7 months ago

matthias-luger commented 7 months ago

Apparently after coflcoin transactions and/or while multiple tabs are open, the coflcoins update weirdly. Has most likely something to do with the 'subEvent' function

https://discord.com/channels/267680588666896385/1219735653281959996

EzequielDM commented 7 months ago

Probably comes from here

new CustomEvent(CUSTOM_EVENTS.COFLCOIN_UPDATE, { detail: { coflCoins: getCurrentCoflCoins() + Math.round(response.data.data.amount) } })

If getCurrentCoflCoins() updates the amount you'll be adding it again

matthias-luger commented 7 months ago

getCurrentCoflCoins returns the current CoflCoins. I add the change to the current CoflCoins and then send the Event that the CoflCoins updated (with the new value).

I tried to reproduce it in all the ways described in the issue and couldn't get it to break. It seems to work for me. Maybe it fixed itself with something Ekwav did last week. For now I am gonna close this

EzequielDM commented 7 months ago

This is a data race issue. The tabs, if all active, will all execute the code and all will update the coins and add them. Given enough delay they will stack on top of each other. The event should only set it to the info gathered from an API call and not add it locally, that's the backend's job. Yet this is a very small and inconsequential issue.

matthias-luger commented 7 months ago

@EzequielDM I get the websocket update from the backend. It only tells me how much the CoflCoins changed, not what it currently is. I also don't 100% like that, but thats how it is.

How exactly is this a race condition? I get the websocket event (in each tab seperatly) and update the local state of the coflcoins (for example by +100). This happens individually for each tab, as the state is not shared. The coflcoins are not stored anywhere where multiple tabs would access it (like localStorage or cookies).

EzequielDM commented 7 months ago

Doesn't the return value of getCurrentCoflCoins get updated?

EDIT: I also couldn't reproduce the issue but it might be because I'm not using the dev env (ain't sure if there's one)

EDIT: oh I see, I should've read a bit more of the actions, the way it works should not cause data racing nor this issue in specific. The value returned from getCurrentCoflCoins refers to a local value and not an action that reads the value from an API

matthias-luger commented 7 months ago

No, it should not.

getCurrentCoflCoins returns a local variable that I can access from all my components (but the variable is also not shared by tabs). This variable gets updated at 2 places.

  1. In the initCoflCoinBalanceAndSubscriptions. This is called once after the user gets logged in and all the data is loaded. This is where it is initially set to the correct amount. So even if this would be called multiple times, it would set it to the correct value.

  2. When the CUSTOM_EVENTS.COFLCOIN_UPDATE is triggered. This sets the new value and tells all components who are registered to get notified on a change. I trigger this event when I get a 'purchase' or 'topup' event from the websocket with the updated value. So when I call getCurrentCoflCoins at this place, I still get the "old" value, as I only send the event after I calculated the new value.

EzequielDM commented 7 months ago

No, it should not.

getCurrentCoflCoins returns a local variable that I can access from all my components (but the variable is also not shared by tabs). This variable gets updated at 2 places.

  1. In the initCoflCoinBalanceAndSubscriptions. This is called once after the user gets logged in and all the data is loaded. This is where it is initially set to the correct amount. So even if this would be called multiple times, it would set it to the correct value.

  2. When the CUSTOM_EVENTS.COFLCOIN_UPDATE is triggered. This sets the new value and tells all components who are registered to get notified on a change. I trigger this event when I get a 'purchase' or 'topup' event from the websocket with the updated value. So when I call getCurrentCoflCoins at this place, I still get the "old" value, as I only send the event after I calculated the new value.

Yep, just skimmed through the code a bit more. This could only happen if the other tabs are notified of such change by the React State changing. I can't seem to get that to happen but changing it to a set function instead of adding should fix whatever is causing this.

matthias-luger commented 7 months ago

Yes, correct. This would only happen if the state would be shared between tabs, which they are not. The only idea I could come up with is that the websocket message with the event was sent more than once and thats why it happened. If this was the case it is now fixed and works.

And yeah, I would also prefer the event to have the current amount of colfcoins and suggested this to Ekwav, but it seems that this won't get added (at least for now).

But as it seems to work now I am fine with it :)

EzequielDM commented 7 months ago

Yeah, on the frontend I can't see anything that would cause that and the fact I can't reproduce it by faking the update must mean it's not an issue.