Prevent segfaults in ProcessWander when docking after flying around #298
Conversation
…emEntities from every bubble
Reviewer's Guide by SourceryThis pull request addresses segfaults in the Sequence diagram for entity removal processsequenceDiagram
participant BubbleManager
participant SystemBubble
participant SystemEntity
BubbleManager->>SystemBubble: Remove(SystemEntity)
loop for each bubble
SystemBubble->>SystemBubble: Untrack(SystemEntity)
end
SystemBubble->>SystemEntity: Set m_bubble to nullptr
Updated class diagram for SystemBubble and BubbleManagerclassDiagram
class SystemBubble {
+void Add(SystemEntity* pSE)
+void Remove(SystemEntity* pSE)
+void Untrack(SystemEntity* pSE)
+void ProcessWander(std::vector<SystemEntity*>& wanderers)
}
class BubbleManager {
+void Remove(SystemEntity* ent)
}
class SystemEntity {
+uint32 GetID()
+bool HasPilot()
+Client* GetPilot()
+bool IsDroneSE()
+SystemBubble* SysBubble()
}
SystemBubble --> SystemEntity
BubbleManager --> SystemBubble
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @charles-m-knox - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider optimizing the entity removal process by implementing a reverse lookup mechanism to quickly find all bubbles an entity is in, rather than iterating over all bubbles in the system. This could improve performance, especially in systems with many bubbles.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // verifies that each entity is still in this bubble. | ||
| // if any entity is no longer in the bubble, they are removed | ||
| // from the bubble and stuck into the vector for re-classification. | ||
| void SystemBubble::ProcessWander(std::vector<SystemEntity *> &wanderers) { |
There was a problem hiding this comment.
suggestion: Consider refactoring ProcessWander into smaller functions
The ProcessWander method has become quite long and complex. Consider breaking it down into smaller, more focused functions to improve readability and maintainability.
void SystemBubble::ProcessWander(std::vector<SystemEntity *> &wanderers) {
for (auto* wanderer : wanderers) {
ProcessSingleWanderer(wanderer);
}
}
void SystemBubble::ProcessSingleWanderer(SystemEntity* wanderer) {
// Implementation details for processing a single wanderer
}
|
|
||
| // iterate through all other bubbles and determine if the entity is | ||
| // in them. | ||
| std::list<SystemBubble *>::iterator itr = m_bubbles.begin(); |
There was a problem hiding this comment.
suggestion: Consider using range-based for loop for better readability
The iteration through m_bubbles could be simplified using a range-based for loop, which would improve readability and reduce the chance of iterator-related errors.
for (SystemBubble* bubble : m_bubbles) {
if (bubble == nullptr) {
| @@ -196,10 +197,37 @@ void BubbleManager::NewBubbleCenter(GVector shipVelocity, GPoint &newCenter) { | |||
|
|
|||
| void BubbleManager::Remove(SystemEntity *ent) { | |||
| // suns, planets and moons arent in bubbles | |||
| //if (ent->IsStaticEntity()) | |||
| // if (ent->IsStaticEntity()) | |||
| // return; | |||
| if (ent->SysBubble() != nullptr) { | |||
There was a problem hiding this comment.
issue (complexity): Consider implementing a reverse lookup mechanism for entity-bubble associations.
The changes to the Remove function in BubbleManager introduce unnecessary complexity and potential performance issues. While the new code ensures an entity is untracked from all bubbles, iterating through all bubbles for each removal operation is inefficient.
Consider optimizing this approach by maintaining a reverse lookup of which bubbles an entity is in. This would allow direct access to relevant bubbles without iterating over all of them. Here's a suggested implementation:
class SystemEntity {
// ... existing code ...
std::set<SystemBubble*> m_bubbles; // Set of bubbles this entity is in
public:
void AddBubble(SystemBubble* bubble) { m_bubbles.insert(bubble); }
void RemoveBubble(SystemBubble* bubble) { m_bubbles.erase(bubble); }
const std::set<SystemBubble*>& GetBubbles() const { return m_bubbles; }
};
void BubbleManager::Remove(SystemEntity *ent) {
if (ent->SysBubble() != nullptr) {
_log(DESTINY__BUBBLE_DEBUG, "BubbleManager::Remove(): Entity %s(%u) being removed from Bubble %u",
ent->GetName(), ent->GetID(), ent->SysBubble()->GetID());
// Untrack from all bubbles the entity is in
for (SystemBubble* bubble : ent->GetBubbles()) {
_log(DESTINY__BUBBLE_DEBUG, "BubbleManager::Remove(): Entity %s(%u) being untracked from Bubble %u",
ent->GetName(), ent->GetID(), bubble->GetID());
bubble->Untrack(ent);
}
ent->SysBubble()->Remove(ent);
}
}This approach maintains the correctness of untracking from all relevant bubbles while avoiding the performance penalty of iterating over all bubbles in the system. Remember to update other parts of the code (like SystemBubble::Add) to maintain this new data structure.
|
Need to close this for now. Seeing something in the logs that I want to investigate a little more. |
This PR prevents segfaults in
SystemBubble::ProcessWanderby untracking soon-to-be-destroyed (i.e. docking) SystemEntities from every bubble they were in recently, not just the bubble they currently reside within.Explanation
After implementing the fixes in #295, a new bug was rearing its head. Every time I would dock after warping somewhere, the server would segfault within about 1 minute.
Turns out that
SystemBubble::ProcessWanderwas what was triggering the segfault. It gets called every minute to clean up entities that are no longer present in each bubble.ProcessWanderiterates over a map, with each key/value pair containing an entity ID and the pointer to the underlyingSystemEntity. It iterates over each entry in the map, attempting to ascertain whether or not the entity should be considered a "wanderer". Such examples of non-wanderers include certain celestials, wrecks, or wormholes, for example.A player's ship is considered a wanderer frequently, because each tick during warp (introduced in #295) generates a new Bubble. In order to clean up the bubbles,
ProcessWanderneeds to first determine who is still within it and remove the entities until each bubble has no entities in its map, which allows the bubble to be deleted later.The segfault thus occurs when the player docks, but has also recently warped. When the player docks,
BubbleManager::Removeis called on the player'sSystemEntity, but unfortunately, it only removes the entity from its current bubble.The player's
SystemEntitypointer is safely deleted as part of this process, but the reason for the segfault is that all of the other bubbles that the ship recently warped through still contain pointers in their mappings of entities they each think they still have within their own bubble.Resolution
The
BubbleManager::Removemethod has been updated to iterate over every single bubble, removing theSystemEntitybefore proceeding to remove its final Bubble. This is facilitated via the newSystemBubble::Untrackmethod that is identical toSystemBubble::Remove, but it omits the final call to set theSystemEntity'sm_bubblepointer to anullptr.Testing
So far the fix seems stable. I'm able to fly around to various spots, dock, undock, etc.
Other changes
Any other changes are only either autoformatter changes or more logging statements.
Summary by Sourcery
Prevent segfaults in SystemBubble::ProcessWander by untracking SystemEntities from all bubbles they were recently in. Introduce a new method, SystemBubble::Untrack, to facilitate this process without unsetting the SystemEntity's bubble. Enhance logging for better traceability.
Bug Fixes:
Enhancements: