Conversation
…n issue with accessing static global variable
|
Caution Review failedThe pull request is closed. WalkthroughAdds a thread-safe IdFactory and ObjectId wrapper, introduces object_id fields for Player and ItemObject, migrates many packet serializers and tests to use get_object_id(), changes item storage iteration to HashMap, and adjusts several player/party/quest method signatures and usages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
login/src/enums.rs (1)
3-23: Prefertracinglogging overprintln!in state transitionsSince
tracing::{error, info}is now imported, consider logging the illegal transition viaerror!instead ofprintln!to keep diagnostics consistent:use l2_core::shared_packets::common::GSLoginFailReasons; use strum::Display; -use tracing::{error, info}; +use tracing::{error, info}; @@ match (&self, desired_state) { @@ - (Self::BfConnected, Self::Authed) => *self = Self::Authed, - _ => { - println!("Can't transition from connection state {self} to {desired_state}"); - return Err(GSLoginFailReasons::NotAuthed); - } + (Self::BfConnected, Self::Authed) => *self = Self::Authed, + _ => { + error!( + "Can't transition from connection state {:?} to {:?}", + self, desired_state + ); + return Err(GSLoginFailReasons::NotAuthed); + } }Please ensure
tracingis properly initialized in the login binary so these logs are actually emitted.game/src/packets/to_client/relation_changed.rs (1)
45-53: Use ofget_object_id()is correct; consider avoiding name shadowingSwitching
obj_idtoplayer.get_object_id()aligns this packet with the new object‑ID mechanism and looks correct.As a small readability tweak, the local
let relation = Relation { ... }currently shadows therelation: u32parameter. Consider renaming the local (e.g.,rel_entryorrelation_entry) to avoid shadowing.l2-core/src/game_objects/item/_item.rs (1)
9-9: Unused import:ArcThe
Arcimport was added but doesn't appear to be used in the changed code within this file. If it's not needed elsewhere in the file (the full context shows no usage), consider removing it.-use std::sync::Arc;l2-core/src/game_objects/player/paper_doll.rs (1)
97-97: Consider using the importedHashMaptype alias.The function signature uses the fully qualified
std::collections::HashMappath. SinceHashMapis a common type, consider addinguse std::collections::HashMap;at the top of the file for cleaner signatures.use entities::dao::item::{ItemVariables, LocType}; use entities::entities::item; +use std::collections::HashMap;Then update the signature:
- pub fn restore_visible_inventory(items: &std::collections::HashMap<i32, crate::game_objects::item::ItemObject>) -> [[i32; 4]; 33] { + pub fn restore_visible_inventory(items: &HashMap<i32, crate::game_objects::item::ItemObject>) -> [[i32; 4]; 33] {l2-core/src/game_objects/player/_player.rs (1)
806-808: Unnecessary separate impl block.This creates a second
impl Playerblock but contains no new constraints or generics. Consider merging with the existingimpl Playerblock above (lines 73-806) for better code organization.l2-core/src/id_factory.rs (1)
102-143: Tests share global singleton state without reset.Since
IdFactory::instance()returns a global singleton, test execution order affects outcomes. The#[serial_test::serial]attribute prevents parallel execution, but accumulated state from previous tests can cause failures when tests run in different orders or in CI.Consider adding a test-only reset method or using a test-scoped factory instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.toml(1 hunks)game/src/packets/from_client/restart.rs(1 hunks)game/src/packets/to_client/char_info.rs(1 hunks)game/src/packets/to_client/char_move_to_location.rs(1 hunks)game/src/packets/to_client/char_selected.rs(3 hunks)game/src/packets/to_client/extended/equipped_items.rs(3 hunks)game/src/packets/to_client/extended/inventory_weight.rs(3 hunks)game/src/packets/to_client/extended/premium_state.rs(3 hunks)game/src/packets/to_client/extended/rotation.rs(3 hunks)game/src/packets/to_client/move_to.rs(1 hunks)game/src/packets/to_client/quest_list.rs(2 hunks)game/src/packets/to_client/relation_changed.rs(1 hunks)game/src/packets/to_client/skill_list.rs(0 hunks)game/src/packets/to_client/user_info.rs(5 hunks)l2-core/Cargo.toml(1 hunks)l2-core/src/data/char_template.rs(1 hunks)l2-core/src/game_objects/item/_item.rs(3 hunks)l2-core/src/game_objects/player/_player.rs(8 hunks)l2-core/src/game_objects/player/paper_doll.rs(1 hunks)l2-core/src/game_objects/player/party.rs(1 hunks)l2-core/src/game_objects/player/quest.rs(1 hunks)l2-core/src/id_factory.rs(1 hunks)l2-core/src/lib.rs(1 hunks)login/src/enums.rs(1 hunks)login/src/packet/handleable/player_logout.rs(1 hunks)
💤 Files with no reviewable changes (1)
- game/src/packets/to_client/skill_list.rs
🧰 Additional context used
🧬 Code graph analysis (10)
game/src/packets/from_client/restart.rs (2)
game/src/packets/to_client/char_selected.rs (1)
new(16-56)game/src/packets/to_client/delete_object.rs (1)
new(13-21)
game/src/packets/to_client/extended/rotation.rs (2)
l2-core/src/id_factory.rs (1)
new(18-20)l2-core/src/game_objects/player/_player.rs (1)
new(77-116)
game/src/packets/to_client/quest_list.rs (1)
l2-core/src/id_factory.rs (1)
new(18-20)
game/src/packets/to_client/extended/inventory_weight.rs (2)
game/src/packets/to_client/extended/premium_state.rs (1)
new(14-21)l2-core/src/game_objects/player/_player.rs (1)
new(77-116)
l2-core/src/id_factory.rs (1)
l2-core/src/game_objects/player/_player.rs (3)
new(77-116)from(37-43)from(46-48)
l2-core/src/game_objects/item/_item.rs (1)
l2-core/src/id_factory.rs (1)
instance(63-73)
game/src/packets/to_client/user_info.rs (1)
l2-core/src/game_objects/player/_player.rs (1)
new(77-116)
game/src/packets/to_client/extended/equipped_items.rs (4)
l2-core/src/id_factory.rs (1)
new(18-20)game/src/packets/to_client/extended/premium_state.rs (1)
new(14-21)game/src/packets/to_client/extended/rotation.rs (1)
new(15-24)game/src/packets/to_client/char_info.rs (1)
new(47-198)
login/src/packet/handleable/player_logout.rs (1)
login/src/controller/player_management.rs (2)
on_players_in_game(94-112)i(66-66)
game/src/packets/to_client/char_selected.rs (3)
l2-core/src/id_factory.rs (1)
new(18-20)l2-core/src/game_objects/player/_player.rs (1)
new(77-116)game/src/packets/to_client/char_selection.rs (1)
new(21-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-only
- GitHub Check: test-only
🔇 Additional comments (30)
l2-core/src/data/char_template.rs (1)
142-144: Base max HP/MP/CP bindings refactor is safeInlining the three
get_base_max_parametercalls into single-lineletbindings preserves behavior and keeps error handling unchanged; looks good.game/src/packets/to_client/char_info.rs (1)
58-58: Usingget_object_id()inCharInfomatches the new ID modelWriting the object ID via
p.get_object_id()(rather than reaching intochar_model.id) is consistent with the new ID factory/ObjectId flow and matches how other packets identify the in‑world object.l2-core/src/lib.rs (1)
25-25: Exposingid_factoryat the crate root is appropriateMaking
id_factorya public module here is a clean way to surface the new Id/IdFactory functionality to the rest of the codebase.game/src/packets/to_client/move_to.rs (1)
17-17:MoveTonow usingget_object_id()is consistent with the new ID schemeUsing
p.get_object_id()for the moving object's identifier keeps this packet in line with the centralized ObjectId/IdFactory approach; no issues from this change.Cargo.toml (1)
63-63: Workspaceserial_testdependency addition is valid and actively usedVerification confirms
serial_test = "3.2.0"is properly declared and actively used in l2-core/src/id_factory.rs (3 test functions with#[serial_test::serial]attributes). Version 3.2.0 is legitimate, MIT-licensed, and published on crates.io with MSRV 1.68.2.game/src/packets/to_client/quest_list.rs (1)
51-72: Deterministic ObjectId usage in test looks goodImporting
ObjectIdand explicitly settingplayer.object_idmakes the test independent of the globalIdFactorystate and fits the new object‑ID model. No issues from a correctness standpoint.login/src/packet/handleable/player_logout.rs (1)
45-52: Usingstd::slice::from_refis correct and efficient
on_players_in_gameexpects&[String], andstd::slice::from_ref(&acc)satisfies that without allocating a newVec. Because the controller clonesacc_nameinternally, there are no lifetime or ownership issues.game/src/packets/to_client/char_move_to_location.rs (1)
16-31: CharMoveToLocation now correctly uses object_idWriting
p.get_object_id()into the packet keeps this serializer consistent with the rest of the protocol, which now keys entities by object ID rather than the character model ID. The rest of the payload remains unchanged and correct.game/src/packets/to_client/extended/rotation.rs (1)
15-24: Rotation packet correctly migrated to object_id with matching test
Rotation::newnow writesplayer.get_object_id(), and the test fixesplayer.object_idviaObjectId::new(268_476_210)and adjusts the expected bytes accordingly. The asserted buffer slice encodes both the object ID and heading in little‑endian as expected, so the test gives good coverage of the new behavior.Also applies to: 30-64
game/src/packets/to_client/extended/premium_state.rs (1)
14-21: PremiumState now uses object_id consistently; test is in syncSwitching to
p.get_object_id()for the identifier, and explicitly settingplayer.object_idin the test, brings this packet in line with the new ID model. The updated expected payload matches the configured object ID, so the test correctly validates the new serialization.Also applies to: 33-34, 55-60
game/src/packets/to_client/extended/inventory_weight.rs (1)
14-24: InventoryWeight packet correctly switched to object_id and test cleaned upUsing
p.get_object_id()for the identifier and assigningplayer.object_idin the test aligns this packet with the new object‑ID system. The updated expected bytes correctly encode the packet and object ID, and droppingmutfromcharremoves an unnecessary mutability marker.Also applies to: 36-37, 42-47, 56-61
game/src/packets/to_client/char_selected.rs (1)
16-26: CharSelected now serializes the correct object_idWriting
player.get_object_id()instead of the character model ID ensuresCharSelectedcarries the runtime object identifier used elsewhere in the protocol. The test’s explicitObjectId::new(268_476_207)and adjusted expected bytes (47, 159, 0, 16) correctly validate this behavior.Also applies to: 64-65, 89-91, 94-104
game/src/packets/from_client/restart.rs (1)
34-39: Restart flow now deletes the correct object_idPassing
player.get_object_id()intoDeleteObject::newmakes the restart broadcast target the same object ID the client uses for that character, instead of the model/DB ID. This is consistent with the rest of the refactor and should prevent “ghost” objects on other clients.l2-core/src/game_objects/item/_item.rs (1)
43-65: LGTM! Clean integration of ObjectId into ItemObject.The
object_idfield addition and the updatedfrom_itemslogic correctly generate unique IDs viaIdFactory::instance().get_next_id()for each item. The HashMap key now uses the object_id value, which aligns with the broader migration to object-based identifiers.l2-core/src/game_objects/player/quest.rs (1)
14-19: LGTM! Clean refactor usingis_some_and().The simplified logic using
.is_some_and(|state| state.eq(expected_state))is more idiomatic and eliminates the need for separate boolean handling. The behavior remains equivalent.l2-core/src/game_objects/player/party.rs (1)
44-61: LGTM! Consistent migration to object_id-based identity.Both
get_leader_id()andindex_of()now correctly useget_object_id()for player identity comparisons, aligning with the codebase-wide transition fromchar_model.idto the new object ID system. ThePartialEqimplementation (lines 13-17) that relies onget_leader_id()remains consistent.game/src/packets/to_client/user_info.rs (4)
55-55: LGTM! Correct migration toget_object_id()for packet serialization.The packet header now correctly uses
player.get_object_id()instead of directly accessingchar_model.id, aligning with the new object identity system.
59-67: Verify: Mixed usage ofchar_model.idandget_object_id().Line 64 still uses
player.char_model.idfor theis_clan_leadercheck while other identity comparisons now useget_object_id(). This may be intentional ifis_clan_leaderrequires the persistent database ID for clan membership lookups, but please verify this is the intended behavior and not an oversight in the migration.
426-431: LGTM! Consistent party leader comparison.The relation logic correctly compares
pt.get_leader_id()withp.get_object_id()for determining party leadership, consistent with the changes inparty.rs.
495-520: LGTM! Test correctly updated for ObjectId.The test now properly initializes
player.object_idwith a specificObjectIdvalue and the expected byte array is updated to match ([44, 159, 0, 16]=268_476_204in little-endian i32).game/src/packets/to_client/extended/equipped_items.rs (2)
24-24: LGTM! Consistent with other packet serialization changes.The packet correctly uses
p.get_object_id()for writing the player's identity, matching the patterns inpremium_state.rs,rotation.rs, andchar_info.rs.
85-90: LGTM! Test correctly updated for ObjectId.The test properly sets
player.object_id = ObjectId::new(268_476_206)and the expected byte array reflects this value correctly ([46, 159, 0, 16]= little-endian representation of268_476_206).l2-core/src/game_objects/player/paper_doll.rs (1)
99-117: LGTM! Correctly adapted for the new ItemObject structure.The iteration and field access patterns are correctly updated:
items.values()iterates over the HashMap's ItemObject valuesitem.item_model.locanditem.item_model.loc_dataaccess the nested modelitem.object_id.value()correctly retrieves the i32 ID for the result arrayThe visual ID extraction and override logic at lines 105-115 is preserved correctly.
l2-core/src/game_objects/player/_player.rs (5)
20-20: LGTM!Import addition for
IdFactoryandObjectIdaligns with the centralized ID management system.
51-53: LGTM!Adding
object_id: ObjectIdas a public field onPlayeris consistent with the new identity model. SinceObjectIdderivesClone,Playercan still deriveClone.
82-87: LGTM!The refactored construction flow is well-structured: creating
Inventoryfrom items first, then restoringPaperDollvisibility based on inventory items, and finally obtaining a uniqueobject_idfrom the singleton factory.Also applies to: 114-114
152-154: Semantic shift from persistent to transient ID.This changes the identity model from
char_model.id(database primary key, persistent) toobject_id(runtime-generated, recycled). Ensure all call sites expecting a persistent character ID are updated to usechar_model.iddirectly if persistence is required (e.g., for database operations or cross-session references).
890-892: LGTM!Using
self.get_object_id()for party index lookup is consistent with the new identity model.l2-core/src/id_factory.rs (2)
48-55: LGTM!The
Dropimplementation correctly checksArc::strong_count(&self.0) == 1before releasing. Since we're insidedrop, we exclusively hold this reference—if the count is 1, no other clones exist, making it safe to release the ID back to the pool.
62-73: LGTM!The singleton pattern using
OnceLockis correct and thread-safe.
…n issue with accessing static global variable
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
l2-core/src/id_factory.rs (1)
63-83: Allocator logic and concurrency look sound; consider long‑term overflow and contention trade‑offsThe singleton +
AtomicI32+Mutex<HashSet<i32>>combination gives you a straightforward, race‑free allocator:get_next_idnow removes from the reuse set while holding the lock, so the TOCTOU race from the earlier version is gone, andfetch_addonnext_idguarantees uniqueness across threads.Two non‑blocking points to consider:
Potential
i32overflow
If this factory is used on a long‑running server with a very high allocation rate and relatively little reuse,next_id: AtomicI32starting atFIRST_OIDcan eventually wrap. That’s very far out in practice, but if you want to be future‑proof you could:
- Switch to
AtomicI64andi64IDs, or- Add a checked upper bound with a clear panic/log when exhausted.
Global mutex contention vs.
DashSet
Moving from a sharded structure toMutex<HashSet<_>>simplifies correctness, but also serializes all reuse operations on a single lock. If ID churn ever becomes a hot path, you might eventually want to revisit a lock‑free or sharded design (e.g.DashSet) with atomic “pop” semantics to avoid pool races, instead of putting everything behind oneMutex. For now this is fine, but worth being aware of the trade‑off.Also applies to: 85-95
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
l2-core/src/id_factory.rs (1)
1-1: Remove unusedDashSetimport.
DashSetis imported but never used. The implementation usesMutex<HashSet<i32>>instead (line 12).-use dashmap::DashSet; use log::warn;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
l2-core/Cargo.toml(1 hunks)l2-core/src/id_factory.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
l2-core/src/id_factory.rs (1)
l2-core/src/game_objects/player/_player.rs (3)
new(77-116)from(37-43)from(46-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-only
🔇 Additional comments (6)
l2-core/src/id_factory.rs (5)
7-15: LGTM! Well-designed ID management types.The constant, struct definitions, and trait derivations are appropriate:
FIRST_OIDprovides a clear starting boundary for object IDsAtomicI32andMutex<HashSet<i32>>ensure thread-safe ID allocation and reuseObjectIdwrappingArc<i32>enables shared ownership with automatic release tracking
17-61: LGTM! Comprehensive trait implementations.The
ObjectIdimplementation and trait impls are well-designed:
#[must_use]attributes prevent accidental misuse- Bidirectional
PartialEqandPartialOrdwithi32enable ergonomic comparisons- The
Dropimplementation correctly checksArc::strong_count == 1before releasing, ensuring IDs are only returned to the pool when the last reference is dropped
64-74: LGTM! Thread-safe singleton pattern.The singleton implementation using
OnceLockis correct and provides thread-safe lazy initialization. ReturningArcclones enables safe shared access across the application.
76-90: LGTM! Race condition successfully resolved.The
get_next_idimplementation now correctly holds the mutex lock throughout the entire read-remove operation (lines 77-82), eliminating the TOCTOU race condition flagged in previous reviews. Therelease_idmethod appropriately warns on double-release attempts.
98-144: LGTM! Test isolation issues successfully resolved.The tests now implement the
reset_for_tests()helper suggested in previous reviews and call it at the start of each test (lines 112, 122, 136). This ensures deterministic behavior by:
- Clearing the reusable ID pool
- Resetting the ID counter to
FIRST_OID- Eliminating dependencies on global state and HashSet iteration order
The test assertions have also been improved to verify specific behaviors (e.g., checking
contains()in line 130) rather than relying on ordering guarantees.l2-core/Cargo.toml (1)
44-45: Theserial_testremoval is confirmed to be intentional. No usages ofserial_testwere found anywhere in thel2-coretests, confirming that the migration to usingreset_for_tests()is complete and the dependency can be safely removed.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.