Check restriction status only on cache

[games647]

TL;DR Registration status will only be checked if the data source is cached to prevent blocking SQL queries on the main thread. Another solution would be a plugin wide notification system when we query the player status, but it's more complex. See more details below at Refresh events.

@ljacqu, I tried to preserve your behavior from this comment. Therefore the proposed changes will still show the inventory for unregistered players, however only if we use the local cache.

Commit desc:

In #2112, we found out that in configurations with disabled cache (like Bungee), this adapter will perform blocking queries on the main thread which impacts the performance.

The original code (81cf14f) is in place to
preserve the inventory for unregistered and configurations without enforcing a registration (#1830, #1752). Alternatives are:

1. Send the inventory on registration like p.updateInventory()

• Still hides it before for unregistered

2. Checking for the enforce registration setting would mean it hides also the inventory for unregistered players
3. Get a notification on player status changes

• Requires a complex setup to propagate the changes across spigot servers
  or different solution for web interfaces
• Refresh events, when the status is updated within the plugin itself would be possible, however requires previous queries like registration/login requests. Instant reports about registration will still be complicated. Example: Every time we make queries, save the result locally and fire an event for our components to check for potential changes without making the same query again. Nevertheless this again delays changes if there is no need to that.

So the best solution is to use the cache if available and if not fallback to safe defaults.

[games647]

Fixes #2112

[ljacqu]

Very pragmatic, looks great to me! Thanks

[games647]

Okay it's not that easy. I forgot the following case:

DataSource not cached, registration not enforced:
Currently: It would always hide the inventory, because we don't know the registration status
Issue: We need to know the status, unregistered are allowed to do anything meanwhile registered needs to be protected.

[games647]

Since the ListenerService is also affected from this issue, I changed to behavior now to use caches there too. Currently the result will be cached on join and updated after login. Only the restriction settings will be using the cache, other components like the register command will bypass it in order to fetch the newest data.

Downside is that changes from other sources like websites won't be available until the player joins the server again. An alternative could be cache every isAuthAvailable if the player is online to have an up to date result when the plugin requests any updates from the database. Furthermore periodic updates like our existing cache (every 5min) would be also possible.

[sgdc3]

Looks good to me

[games647]

I think I should add some unit tests too, because this is critical area. Just to be sure ;)

[sgdc3]

@games647 hello, any news on this?

[games647]

Argh, I was too busy with other stuff. I'll try on Thursday.

[sgdc3]

@games647 re-bump? 😄

[sgdc3]

@games647 re-re-bump 🙃

[games647]

@sgdc3 In fact working on it now.

[games647]

Test are ready. However I encountered a race condition. When preventing the inventory data to be sent the status of the registration status isn't loaded yet. Therefore we should always prevent it at this stage. My issue is creating a good design. Currently we fire a ProtectInventoryEvent only if necessary on player join. My idea would be to always fire it on join and fire RestoreInventoryEvent once data is loaded.

EDIT: However this could cause a change semantics that maybe some plugins depend on.

[sgdc3]

EDIT: However this could cause a change semantics that maybe some plugins depend on.

I see no other solution, it is a minor breaking change but I think the performance gains are more important.

[sgdc3]

@ljacqu do you agree, what's your opinion on this?

[sgdc3]

@games647 @ljacqu bump

[sgdc3]

@games647 @ljacqu re-bump? 😄

[sgdc3]

@games647 hello? 😅