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.

Sign up for GitHub

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

Fix scheduler leaks #4882

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+108 −104
Open

Fix scheduler leaks #4882

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c419a21
fix weapons memory leaks
ArturKnopik Dec 24, 2024
4e0fd25
fix weapons memory leaks
ArturKnopik Dec 24, 2024
ba5a525
fix scheduler/tasks leaks
ArturKnopik Dec 24, 2024
abdf6c5
fix scheduler/tasks leaks
ArturKnopik Dec 24, 2024
32d01a9
fix formating
ArturKnopik Dec 24, 2024
3f23308
review fixes
ArturKnopik Dec 27, 2024
8699451
up
ArturKnopik Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 34 additions & 34 deletions src/game.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,11 +634,11 @@ void Game::playerMoveThing(uint32_t playerId, const Position& fromPos, uint16_t
}

if (movingCreature->getPosition().isInRange(player->getPosition(), 1, 1, 0)) {
SchedulerTask* task = createSchedulerTask(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in these situations we should use auto, because it doesn't even matter what the type is here, we just pass it along. Wdyt?

Copy link
Member

@nekiro nekiro Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just move the creation directly to method call, then we can let the compiler do its best thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or just move the creation directly to method call, then we can let the compiler do its best thing

it's looks like some abomination
Zrzut ekranu z 2024-12-27 09-49-31

Copy link
Member

@nekiro nekiro Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::move shouldnt be needed there, but yeah, might be true, it doesnt look the best

SchedulerTask_ptr task = createSchedulerTask(
MOVE_CREATURE_INTERVAL, [=, this, playerID = player->getID(), creatureID = movingCreature->getID()]() {
playerMoveCreatureByID(playerID, creatureID, fromPos, toPos);
});
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
} else {
playerMoveCreature(player, movingCreature, movingCreature->getPosition(), tile);
}
Expand Down Expand Up @@ -680,12 +680,12 @@ void Game::playerMoveCreature(Player* player, Creature* movingCreature, const Po
{
if (!player->canDoAction()) {
uint32_t delay = player->getNextActionTime();
SchedulerTask* task =
SchedulerTask_ptr task =
createSchedulerTask(delay, [=, this, playerID = player->getID(), movingCreatureID = movingCreature->getID(),
toPos = toTile->getPosition()]() {
playerMoveCreatureByID(playerID, movingCreatureID, movingCreatureOrigPos, toPos);
});
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
return;
}

Expand All @@ -703,13 +703,13 @@ void Game::playerMoveCreature(Player* player, Creature* movingCreature, const Po
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task =
SchedulerTask_ptr task =
createSchedulerTask(RANGE_MOVE_CREATURE_INTERVAL, [=, this, playerID = player->getID(),
movingCreatureID = movingCreature->getID(),
toPos = toTile->getPosition()] {
playerMoveCreatureByID(playerID, movingCreatureID, movingCreatureOrigPos, toPos);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -891,10 +891,10 @@ void Game::playerMoveItem(Player* player, const Position& fromPos, uint16_t spri
{
if (!player->canDoAction()) {
uint32_t delay = player->getNextActionTime();
SchedulerTask* task = createSchedulerTask(delay, [=, this, playerID = player->getID()]() {
SchedulerTask_ptr task = createSchedulerTask(delay, [=, this, playerID = player->getID()]() {
playerMoveItemByPlayerID(playerID, fromPos, spriteId, fromStackPos, toPos, count);
});
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
return;
}

Expand Down Expand Up @@ -960,11 +960,11 @@ void Game::playerMoveItem(Player* player, const Position& fromPos, uint16_t spri
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task =
SchedulerTask_ptr task =
createSchedulerTask(RANGE_MOVE_ITEM_INTERVAL, [=, this, playerID = player->getID()]() {
playerMoveItemByPlayerID(playerID, fromPos, spriteId, fromStackPos, toPos, count);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -1022,12 +1022,12 @@ void Game::playerMoveItem(Player* player, const Position& fromPos, uint16_t spri
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
RANGE_MOVE_ITEM_INTERVAL,
[this, playerID = player->getID(), itemPos, spriteId, itemStackPos, toPos, count]() {
playerMoveItemByPlayerID(playerID, itemPos, spriteId, itemStackPos, toPos, count);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -2137,10 +2137,10 @@ void Game::playerUseItemEx(uint32_t playerId, const Position& fromPos, uint8_t f
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(RANGE_USE_ITEM_EX_INTERVAL, [=, this]() {
SchedulerTask_ptr task = createSchedulerTask(RANGE_USE_ITEM_EX_INTERVAL, [=, this]() {
playerUseItemEx(playerId, itemPos, itemStackPos, fromSpriteId, toPos, toStackPos, toSpriteId);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand All @@ -2153,10 +2153,10 @@ void Game::playerUseItemEx(uint32_t playerId, const Position& fromPos, uint8_t f

if (!player->canDoAction()) {
uint32_t delay = player->getNextActionTime();
SchedulerTask* task = createSchedulerTask(delay, [=, this]() {
SchedulerTask_ptr task = createSchedulerTask(delay, [=, this]() {
playerUseItemEx(playerId, fromPos, fromStackPos, fromSpriteId, toPos, toStackPos, toSpriteId);
});
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
return;
}

Expand Down Expand Up @@ -2198,9 +2198,9 @@ void Game::playerUseItem(uint32_t playerId, const Position& pos, uint8_t stackPo
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
RANGE_USE_ITEM_INTERVAL, [=, this]() { playerUseItem(playerId, pos, stackPos, index, spriteId); });
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
return;
}

Expand All @@ -2213,9 +2213,9 @@ void Game::playerUseItem(uint32_t playerId, const Position& pos, uint8_t stackPo

if (!player->canDoAction()) {
uint32_t delay = player->getNextActionTime();
SchedulerTask* task =
SchedulerTask_ptr task =
createSchedulerTask(delay, [=, this]() { playerUseItem(playerId, pos, stackPos, index, spriteId); });
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
return;
}

Expand Down Expand Up @@ -2297,10 +2297,10 @@ void Game::playerUseWithCreature(uint32_t playerId, const Position& fromPos, uin
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(RANGE_USE_WITH_CREATURE_INTERVAL, [=, this]() {
SchedulerTask_ptr task = createSchedulerTask(RANGE_USE_WITH_CREATURE_INTERVAL, [=, this]() {
playerUseWithCreature(playerId, itemPos, itemStackPos, creatureId, spriteId);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand All @@ -2313,9 +2313,9 @@ void Game::playerUseWithCreature(uint32_t playerId, const Position& fromPos, uin

if (!player->canDoAction()) {
uint32_t delay = player->getNextActionTime();
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
delay, [=, this]() { playerUseWithCreature(playerId, fromPos, fromStackPos, creatureId, spriteId); });
player->setNextActionTask(task);
player->setNextActionTask(std::move(task));
return;
}

Expand Down Expand Up @@ -2416,9 +2416,9 @@ void Game::playerRotateItem(uint32_t playerId, const Position& pos, uint8_t stac
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
RANGE_ROTATE_ITEM_INTERVAL, [=, this]() { playerRotateItem(playerId, pos, stackPos, spriteId); });
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -2512,9 +2512,9 @@ void Game::playerBrowseField(uint32_t playerId, const Position& pos)
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task =
SchedulerTask_ptr task =
createSchedulerTask(RANGE_BROWSE_FIELD_INTERVAL, [=, this]() { playerBrowseField(playerId, pos); });
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -2618,9 +2618,9 @@ void Game::playerWrapItem(uint32_t playerId, const Position& position, uint8_t s
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
RANGE_WRAP_ITEM_INTERVAL, [=, this]() { playerWrapItem(playerId, position, stackPos, spriteId); });
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -2690,10 +2690,10 @@ void Game::playerRequestTrade(uint32_t playerId, const Position& pos, uint8_t st
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(RANGE_REQUEST_TRADE_INTERVAL, [=, this]() {
SchedulerTask_ptr task = createSchedulerTask(RANGE_REQUEST_TRADE_INTERVAL, [=, this]() {
playerRequestTrade(playerId, pos, stackPos, tradePlayerId, spriteId);
});
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down Expand Up @@ -3413,9 +3413,9 @@ void Game::playerRequestEditPodium(uint32_t playerId, const Position& position,
g_dispatcher.addTask([this, playerID = player->getID(), listDir = std::move(listDir)]() {
playerAutoWalk(playerID, listDir);
});
SchedulerTask* task = createSchedulerTask(
SchedulerTask_ptr task = createSchedulerTask(
400, [=, this]() { playerRequestEditPodium(playerId, position, stackPos, spriteId); });
player->setNextWalkActionTask(task);
player->setNextWalkActionTask(std::move(task));
} else {
player->sendCancelMessage(RETURNVALUE_THEREISNOWAY);
}
Expand Down
20 changes: 9 additions & 11 deletions src/player.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,26 +1508,25 @@ void Player::checkTradeState(const Item* item)
}
}

void Player::setNextWalkActionTask(SchedulerTask* task)
void Player::setNextWalkActionTask(SchedulerTask_ptr task)
{
if (walkTaskEvent != 0) {
g_scheduler.stopEvent(walkTaskEvent);
walkTaskEvent = 0;
}

delete walkTask;
walkTask = task;
walkTask = std::move(task);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe if you use

Suggested change
walkTask = std::move(task);
std::swap(walkTask, task);

You may skip one deletion. This is absolutely minor, change it only if you touch this code again 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? deletion will be moved to end of the scope

}

void Player::setNextActionTask(SchedulerTask* task, bool resetIdleTime /*= true */)
void Player::setNextActionTask(SchedulerTask_ptr task, bool resetIdleTime /*= true */)
{
if (actionTaskEvent != 0) {
g_scheduler.stopEvent(actionTaskEvent);
actionTaskEvent = 0;
}

if (task) {
actionTaskEvent = g_scheduler.addEvent(task);
actionTaskEvent = g_scheduler.addEvent(std::move(task));
if (resetIdleTime) {
this->resetIdleTime();
}
Expand Down Expand Up @@ -3407,13 +3406,13 @@ void Player::doAttacking(uint32_t)
result = Weapon::useFist(this, attackedCreature);
}

SchedulerTask* task = createSchedulerTask(std::max<uint32_t>(SCHEDULER_MINTICKS, delay),
[id = getID()]() { g_game.checkCreatureAttack(id); });
SchedulerTask_ptr task = createSchedulerTask(std::max<uint32_t>(SCHEDULER_MINTICKS, delay),
[id = getID()]() { g_game.checkCreatureAttack(id); });
if (!classicSpeed) {
setNextActionTask(task, false);
setNextActionTask(std::move(task), false);
} else {
g_scheduler.stopEvent(classicAttackEvent);
classicAttackEvent = g_scheduler.addEvent(task);
classicAttackEvent = g_scheduler.addEvent(std::move(task));
}

if (result) {
Expand Down Expand Up @@ -3469,8 +3468,7 @@ void Player::onWalkAborted()
void Player::onWalkComplete()
{
if (walkTask) {
walkTaskEvent = g_scheduler.addEvent(walkTask);
walkTask = nullptr;
walkTaskEvent = g_scheduler.addEvent(std::move(walkTask));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prone to use-after-move, you still need walkTask = nullptr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your suggestion is good, good capture, I was thinking about this:

walkTaskEvent = g_scheduler.addEvent(SchedulerTask_ptr(walkTask.release()));

However, I am not convinced about having to build a new unique_ptr, although it should be free, I don't know how profitable it is.

The option to assign nullptr is correct and I like it

}
}

Expand Down
7 changes: 4 additions & 3 deletions src/player.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "guild.h"
#include "inbox.h"
#include "protocolgame.h"
#include "scheduler.h"
#include "town.h"
#include "vocation.h"

Expand Down Expand Up @@ -1135,8 +1136,8 @@ class Player final : public Creature, public Cylinder

void updateInventoryWeight();

void setNextWalkActionTask(SchedulerTask* task);
void setNextActionTask(SchedulerTask* task, bool resetIdleTime = true);
void setNextWalkActionTask(SchedulerTask_ptr task);
void setNextActionTask(SchedulerTask_ptr task, bool resetIdleTime = true);

void death(Creature* lastHitCreature) override;
bool dropCorpse(Creature* lastHitCreature, Creature* mostDamageCreature, bool lastHitUnjustified,
Expand Down Expand Up @@ -1226,7 +1227,7 @@ class Player final : public Creature, public Cylinder
Npc* shopOwner = nullptr;
Party* party = nullptr;
Player* tradePartner = nullptr;
SchedulerTask* walkTask = nullptr;
SchedulerTask_ptr walkTask = nullptr;
const Town* town = nullptr;
Vocation* vocation = nullptr;
StoreInbox* storeInbox = nullptr;
Expand Down
36 changes: 20 additions & 16 deletions src/scheduler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,34 @@

#include "scheduler.h"

uint32_t Scheduler::addEvent(SchedulerTask* task)
uint32_t Scheduler::addEvent(SchedulerTask_ptr task)
{
// check if the event has a valid id
if (task->getEventId() == 0) {
task->setEventId(++lastEventId);
}

boost::asio::post(io_context, [this, task]() {
auto eventId = task->getEventId();

boost::asio::post(io_context, [this, scheduledTask = std::move(task)]() mutable {
// insert the event id in the list of active events
auto it = eventIdTimerMap.emplace(task->getEventId(), boost::asio::steady_timer{io_context});
auto it = eventIdTimerMap.emplace(scheduledTask->getEventId(), boost::asio::steady_timer{io_context});
auto& timer = it.first->second;

timer.expires_after(std::chrono::milliseconds(task->getDelay()));
timer.async_wait([this, task](const boost::system::error_code& error) {
eventIdTimerMap.erase(task->getEventId());
timer.expires_after(std::chrono::milliseconds(scheduledTask->getDelay()));
timer.async_wait(
[this, scheduledTask = std::move(scheduledTask)](const boost::system::error_code& error) mutable {
eventIdTimerMap.erase(scheduledTask->getEventId());

if (error == boost::asio::error::operation_aborted || getState() == THREAD_STATE_TERMINATED) {
// the timer has been manually canceled(timer->cancel()) or Scheduler::shutdown has been called
delete task;
return;
}
if (error == boost::asio::error::operation_aborted || getState() == THREAD_STATE_TERMINATED) {
// The timer was manually canceled or Scheduler::shutdown was called
return;
}

g_dispatcher.addTask(task);
});
g_dispatcher.addTask(std::move(scheduledTask));
});
});

return task->getEventId();
return eventId;
}

void Scheduler::stopEvent(uint32_t eventId)
Expand Down Expand Up @@ -62,4 +63,7 @@ void Scheduler::shutdown()
});
}

SchedulerTask* createSchedulerTask(uint32_t delay, TaskFunc&& f) { return new SchedulerTask(delay, std::move(f)); }
SchedulerTask_ptr createSchedulerTask(uint32_t delay, TaskFunc&& f)
{
return SchedulerTask_ptr(new SchedulerTask(delay, std::move(f)));
MillhioreBT marked this conversation as resolved.
Show resolved Hide resolved
}
10 changes: 7 additions & 3 deletions src/scheduler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@

static constexpr int32_t SCHEDULER_MINTICKS = 50;

class SchedulerTask;

using SchedulerTask_ptr = std::unique_ptr<SchedulerTask>;

class SchedulerTask : public Task
{
public:
Expand All @@ -23,15 +27,15 @@ class SchedulerTask : public Task
uint32_t eventId = 0;
uint32_t delay = 0;

friend SchedulerTask* createSchedulerTask(uint32_t, TaskFunc&&);
friend SchedulerTask_ptr createSchedulerTask(uint32_t delay, TaskFunc&& f);
ranisalt marked this conversation as resolved.
Show resolved Hide resolved
};

SchedulerTask* createSchedulerTask(uint32_t delay, TaskFunc&& f);
SchedulerTask_ptr createSchedulerTask(uint32_t delay, TaskFunc&& f);

class Scheduler : public ThreadHolder<Scheduler>
{
public:
uint32_t addEvent(SchedulerTask* task);
uint32_t addEvent(SchedulerTask_ptr task);
void stopEvent(uint32_t eventId);

void shutdown();
Expand Down
Loading
Loading