Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect instance reset crash fix. #698

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mostlikely4r
Copy link
Contributor

🍰 Pullrequest

This PR fixes crashes when resetting an instance ID that previously belonged to a dungeon but was re-used as a battleground.

Proof

On server start in

sLog.outString("Cleaning up instances...");
instance ID's are first cleaned up before they are 'packed' which means they are lowered to remove any gaps. The problem however is that besides cleaning up this method also starts a reset countdown timer for instances that are slated to reset but have not done so yet. This timer is started for the original ID's and not modified along with the database content during the packing.

The end result is that an instance id 10 which is slated to expire after server start can be lowered to 5 while a new battleground will re-use the 10 which causes a assert. More detail is given in the fixed issue.

While moving the packing in front of the cleaning results in a slightly less efficient packing as some instances will be removed leaving some holes, it is simpler and less error prone than modifying the reset timers during the packing.

Issues

How2Test

Start a few instances during a server and make some expire (by breaking up the groups) such that gaps are created in the instance ID's after server shutdown.
Now before the remaining instances are slated to reset start the server and notice that the original instance ID's are modified in the database to a lower number. Now start battlegrounds such that the original ID's are re-used. Keep the battleground active until the original dungeon was slated to reset and watch the server crash (or not crash with this fix).

@killerwife
Copy link
Contributor

Same as other PR

@mostlikely4r
Copy link
Contributor Author

Same as other PR

Thanks for the tip. Does it look better now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants