forgottenserver icon indicating copy to clipboard operation
forgottenserver copied to clipboard

Fix scheduler leaks

Open ArturKnopik opened this issue 1 year ago • 9 comments

Pull Request Prelude

  • [X] I have followed [proper The Forgotten Server code styling][code].
  • [X] I have read and understood the [contribution guidelines][cont] before making this PR.
  • [X] 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:

https://github.com/otland/forgottenserver/issues/4288

How to test:

Requirements: Linux

  1. Build project in debug mode
  2. Run server using valgrind(valgrind --leak-check=full ./tfs)
  3. Shutdown server
  4. 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== 

ArturKnopik avatar Dec 24 '24 12:12 ArturKnopik

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

ranisalt avatar Dec 24 '24 19:12 ranisalt

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 :)

ArturKnopik avatar Dec 25 '24 13:12 ArturKnopik

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.

MillhioreBT avatar Dec 29 '24 15:12 MillhioreBT

after std::move source ptr is nullified. image

ArturKnopik avatar Dec 30 '24 12:12 ArturKnopik

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.

nekiro avatar Dec 30 '24 14:12 nekiro

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

MillhioreBT avatar Dec 30 '24 16:12 MillhioreBT

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

ArturKnopik avatar Dec 30 '24 17:12 ArturKnopik

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

ranisalt avatar Dec 31 '24 15:12 ranisalt

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 avatar Dec 31 '24 16:12 ArturKnopik

Can someone do a review?

ArturKnopik avatar Jul 01 '25 21:07 ArturKnopik