Fix scheduler leaks #4882

ArturKnopik · 2024-12-24T12:33:23Z

Pull Request Prelude

I have followed [proper The Forgotten Server code styling][code].
I have read and understood the [contribution guidelines][cont] before making this PR.
I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

Fix scheduler memory leaks

Issues addressed:

#4288

How to test:

Requirements: Linux

Build project in debug mode
Run server using valgrind(valgrind --leak-check=full ./tfs)
Shutdown server
Looks for logs like (They shouldn't be there)

==44907== 56 bytes in 1 blocks are definitely lost in loss record 461 of 568
==44907==    at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==44907==    by 0x19D5DA: createSchedulerTask(unsigned int, std::function<void ()>&&) (scheduler.cpp:65)
==44907==    by 0x2DC8B5: IOMarket::checkExpiredOffers() (iomarket.cpp:193)
==44907==    by 0x278E46: (anonymous namespace)::mainLoader(ServiceManager*) (otserv.cpp:248)
==44907==    by 0x2794D6: startServer()::{lambda()#1}::operator()() const (otserv.cpp:285)
==44907==    by 0x27B005: void std::__invoke_impl<void, startServer()::{lambda()#1}&>(std::__invoke_other, startServer()::{lambda()#1}&) (invoke.h:61)
==44907==    by 0x27AD34: std::enable_if<is_invocable_r_v<void, startServer()::{lambda()#1}&>, void>::type std::__invoke_r<void, startServer()::{lambda()#1}&>(startServer()::{lambda()#1}&) (invoke.h:111)
==44907==    by 0x27A772: std::_Function_handler<void (), startServer()::{lambda()#1}>::_M_invoke(std::_Any_data const&) (std_function.h:290)
==44907==    by 0x168943: std::function<void ()>::operator()() const (std_function.h:591)
==44907==    by 0x16097F: Task::operator()() (tasks.h:23)
==44907==    by 0x14DAD0: Dispatcher::threadMain() (tasks.cpp:37)
==44907==    by 0x2D7CA3: void std::__invoke_impl<void, void (Dispatcher::*)(), Dispatcher*>(std::__invoke_memfun_deref, void (Dispatcher::*&&)(), Dispatcher*&&) (invoke.h:74)
==44907==

src/tasks.h

ranisalt · 2024-12-24T19:45:28Z

src/game.cpp

@@ -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(


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?

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

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

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

src/tasks.cpp

ranisalt · 2024-12-24T19:50:54Z

This is really neat, we should do more of that.

I'd love to help more but valgrind borked on my machine and won't run :(

ArturKnopik · 2024-12-25T13:19:58Z

This is really neat, we should do more of that.

I'd love to help more but valgrind borked on my machine and won't run :(

Thanks, you can help by doing a review :)

src/scheduler.cpp

ArturKnopik · 2024-12-27T10:25:40Z

src/scheduler.h

 class SchedulerTask : public Task
 {
 public:
+	SchedulerTask(uint32_t delay, TaskFunc&& f) : Task(std::forward<TaskFunc>(f)), delay(delay) {}


must become public to give abllity to call c-tor by make_unique from SchedulerTask_ptr createSchedulerTask(uint32_t delay, TaskFunc&& f)

you could use a friend class

Friend is here but somehow it's not compile :<

the problem is make_unique becouse it has no access to private c-tor of SchedulerTask class
there is no way to use something like as friend:
template <typename... Args> Friend std::unique_ptr<SchedulerTask> std::make_unique(Args&&... args);

friend sucks, just make it public

MillhioreBT · 2024-12-29T15:58:26Z

Btw, wouldn't it be dangerous to use walkTask after being moved?
for example in the goToFollowCreature method it is used to validate in a boolean context
Using an object that was moved is undefined behavior.

ranisalt · 2024-12-30T00:11:35Z

src/player.cpp

 {
 	if (walkTaskEvent != 0) {
 		g_scheduler.stopEvent(walkTaskEvent);
 		walkTaskEvent = 0;
 	}

-	delete walkTask;
-	walkTask = task;
+	walkTask = std::move(task);


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 😆

why? deletion will be moved to end of the scope

ranisalt · 2024-12-30T00:12:12Z

src/player.cpp

@@ -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));


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

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

src/scheduler.h

ArturKnopik · 2024-12-30T12:45:04Z

after std::move source ptr is nullified.

nekiro · 2024-12-30T14:38:24Z

after std::move source ptr is nullified. ![image](https://private-user-images.githubusercontent.com/17506599/399293282-

I recall reading that this behaviour isn't explicitly predictable to be always nullified. There is no guarantee, so it's not safe to assume, but maybe that wasn't for unique ptr.

MillhioreBT · 2024-12-30T16:54:59Z

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf

section 20.7.1 point 4 Additionally, u can, upon request, transfer ownership to another unique pointer u2. Upon completion of such a transfer, the following postconditions hold: — u2.p is equal to the pre-transfer u.p, — u.p is equal to nullptr, and — if the pre-transfer u.d maintained state, such state has been transferred to u2.d.

what would be the point if after transferring ownership the pointer address remained unchanged... that was at odds with the idea of unique ptr

It is better not to assume anything and do things as safely as possible.
By the way the linter actually warns you not to use objects that were moved, no matter if it is a unique_ptr or something else

ArturKnopik · 2024-12-30T17:06:53Z

https://stackoverflow.com/questions/36071220/what-happens-to-unique-ptr-after-stdmove

ranisalt · 2024-12-31T15:09:50Z

Nice, so this is one of the few use-after-move situations that are defined 😆 good to go

ArturKnopik · 2024-12-31T16:55:28Z

Nice, so this is one of the few use-after-move situations that are defined 😆 good to go

yes, this is one of the few cases where it is defined in the standard

ArturKnopik added 4 commits December 24, 2024 08:50

fix weapons memory leaks

c419a21

fix weapons memory leaks

4e0fd25

fix scheduler/tasks leaks

ba5a525

fix scheduler/tasks leaks

abdf6c5

ArturKnopik requested review from ranisalt, DSpeichert and nekiro December 24, 2024 12:33

fix formating

32d01a9

ArturKnopik changed the title ~~Fix scheaduler leaks~~ Fix scheduler leaks Dec 24, 2024

ranisalt reviewed Dec 24, 2024

View reviewed changes

src/tasks.h Outdated Show resolved Hide resolved

ranisalt reviewed Dec 24, 2024

View reviewed changes

src/tasks.h Outdated Show resolved Hide resolved

ranisalt reviewed Dec 24, 2024

View reviewed changes

src/tasks.cpp Outdated Show resolved Hide resolved

MillhioreBT reviewed Dec 26, 2024

View reviewed changes

src/scheduler.cpp Outdated Show resolved Hide resolved

review fixes

3f23308

ArturKnopik commented Dec 27, 2024

View reviewed changes

ranisalt reviewed Dec 30, 2024

View reviewed changes

src/scheduler.h Outdated Show resolved Hide resolved

up

8699451

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scheduler leaks #4882

Fix scheduler leaks #4882

ArturKnopik commented Dec 24, 2024

ranisalt Dec 24, 2024

nekiro Dec 25, 2024 •

edited

Loading

ArturKnopik Dec 27, 2024

nekiro Dec 27, 2024 •

edited

Loading

ranisalt commented Dec 24, 2024 •

edited

Loading

ArturKnopik commented Dec 25, 2024

ArturKnopik Dec 27, 2024

nekiro Dec 27, 2024

ArturKnopik Dec 27, 2024

ArturKnopik Dec 27, 2024

ranisalt Dec 30, 2024

MillhioreBT commented Dec 29, 2024 •

edited

Loading

ranisalt Dec 30, 2024

ArturKnopik Dec 30, 2024

ranisalt Dec 30, 2024

MillhioreBT Dec 30, 2024

ArturKnopik commented Dec 30, 2024

nekiro commented Dec 30, 2024 •

edited

Loading

MillhioreBT commented Dec 30, 2024

ArturKnopik commented Dec 30, 2024

ranisalt commented Dec 31, 2024

ArturKnopik commented Dec 31, 2024

Fix scheduler leaks #4882

Are you sure you want to change the base?

Fix scheduler leaks #4882

Conversation

ArturKnopik commented Dec 24, 2024

Pull Request Prelude

Changes Proposed

Choose a reason for hiding this comment

nekiro Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekiro Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

ranisalt commented Dec 24, 2024 • edited Loading

ArturKnopik commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MillhioreBT commented Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArturKnopik commented Dec 30, 2024

nekiro commented Dec 30, 2024 • edited Loading

MillhioreBT commented Dec 30, 2024

ArturKnopik commented Dec 30, 2024

ranisalt commented Dec 31, 2024

ArturKnopik commented Dec 31, 2024

nekiro Dec 25, 2024 •

edited

Loading

nekiro Dec 27, 2024 •

edited

Loading

ranisalt commented Dec 24, 2024 •

edited

Loading

MillhioreBT commented Dec 29, 2024 •

edited

Loading

nekiro commented Dec 30, 2024 •

edited

Loading