Fix scheduler leaks
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
- 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==
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 :(
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 :)
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.
after std::move source ptr is nullified.
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.
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
https://stackoverflow.com/questions/36071220/what-happens-to-unique-ptr-after-stdmove
Nice, so this is one of the few use-after-move situations that are defined 😆 good to go
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
Can someone do a review?