Redis queue - #38
Conversation
…ate connection management
…cross-proxy queue syncing
… management and logging
ajgeiss0702
left a comment
ajgeiss0702
left a comment
There was a problem hiding this comment.
A few issues I've noted. I haven't looked very closely at the actual redis logic yet
|
I've made all the requested changes. It's up to you to resolve the threads. I'm now testing the changes to ensure nothing broke in the process, and will edit this comment once reviewed. |
| List<String> list = cmd.lrange(key, 0, -1); | ||
| for (int i = 0; i < list.size(); i++) { | ||
| if (list.get(i).startsWith(uuidPrefix)) { | ||
| cmd.lset(key, i, serialized); |
There was a problem hiding this comment.
The indexes could shift in the middle of this loop being executed (due to another server sending a player), causing duplicated and incorrectly-positioned players.
| */ | ||
| private void rebuildList(RedisCommands<String, String> cmd, String key, List<String> items) { | ||
| cmd.del(key); | ||
| if (!items.isEmpty()) cmd.rpush(key, items.toArray(new String[0])); |
There was a problem hiding this comment.
Two problems (from AI):
- Crash between DEL and RPUSH (network blip, Redis disconnect, command timeout) leaves the queue permanently empty. The whole per-server queue is gone with no recovery. For a "persistent" holder this is the worst possible failure mode.
- Concurrent writes from any other proxy (or this proxy's own onPlayerOffline / addPlayer) are lost. Two proxies each compute their own replacement list from a stale lrange, then DEL+RPUSH overwrite each other. The loser's insertion silently vanishes.
sendPlayers on QueueManagerImpl is synchronized, so on a single proxy two schedulers don't race. But cross-proxy races and process crashes are real.
Recommendation: use MULTI/EXEC (or a single Lua script) to make the replacement atomic, e.g. DEL+RPUSH inside a transaction with WATCH, or use RPUSH-only inserts with a coordinatated server-side script that handles positional insertion atomically. The non-positional addPlayer(player) path (plain RPUSH) is already atomic — prefer it where possible.
| continue; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Is not fail-safe. Two proxies checking the shared timestamp at a close enough time will both read before either one writes. Might be fine since it would be rare (the difference between both proxies' send intervals would need to be less than the network latency) but worth thinking about, and maybe adjusting the comment if keeping this behaviour
| () -> redisClient.connect(), | ||
| poolConfig, | ||
| true // wrapConnections=true: close() returns the connection to the pool | ||
| ); |
There was a problem hiding this comment.
if createGenericObjectPool throws for whatever reason, redisClient will be set but connectionPool will not be, leading to a re-init on next call, which would leak a redisClient.
Maybe don't set these until after both RedisClient.create and ConnectionPoolSupport.createGenericObjectPool succeed without throwing
| } | ||
|
|
||
| // Debug.info("should send when back online: " + !server.isGroup() + " && " + main.getConfig().getBoolean("send-all-when-back-online") + " && " + server.getServers().get(0).justWentOnline()); | ||
| Debug.info("should send when back online: " + !server.isGroup() + " && " + main.getConfig().getBoolean("send-all-when-back-online") + " && " + server.getServers().get(0).justWentOnline()); |
There was a problem hiding this comment.
I usually only uncomment this when I need it, because it spams debug too much
2 participants
I've implemented the Redis queue holder using the recommendations made on #11. I did have to modify the base queue holder in order to add an onOffline method, which was the cleanest option to sync disconnects across instances.
I have not fully tested this, and I won't. But the queue functionality via API (without any modifications needed from dependants) and commands works flawlessly from what I've seen.
I've also removed some unused imports.
Both AI and manual coding was involved.
Tested on Velocity.
This update does not require RedisBungee/ValioBungee to be installed, it is a standalone implementation. It seems the player count is correctly retrieved via the ping server API.
If you want to test it more in depth or review it, I recommend cloning my multi-proxy multi-instance test bench here: https://gitlab.com/purevanilla/services/test-bench - the readme is outdated; this now uses luck perms and you won't have the default perms when joining, but you can just do
docker compose attach proxy-1(after having donedocker compose up -dand then dolpv editorto handle permission nodes (shared across all servers and proxies). Simply place the built jar on .defaults/proxy/plugins along with the config folder, and it will be copied on all proxies withdocker compose up -dand you can call./reset.shto reset the test bench to a blank slate (will remove all data).