Page MenuHomeWildfire Games

Shutdown mongoose in rl-interface gracefully
Needs ReviewPublic

Authored by phosit on Mar 24 2024, 7:54 PM.

Details

Reviewers
vladislavbelov
Trac Tickets
#5989
Summary

When the RL::Interface is destroied but the engine isn't completely shoutdown there was a dangling pointer. The duration the dangling pointer is accessable got expanded with rP27908.
Whith this patch there no dangling pointer (in that regard).

Test Plan

Run with the rl-interface.
Restart the engine while the rl-interface runs (from the mod selector). The rl-interface should also restart

Event Timeline

phosit created this revision.Mar 24 2024, 7:54 PM
phosit published this revision for review.Mar 24 2024, 9:12 PM
phosit retitled this revision from Shutdown rl-interface gracefully to Shutdown mongoose in rl-interface gracefully.Wed, Mar 27, 12:52 PM
In D5254#223622, @Stan wrote:

Yea... fixing a bug while not noticing it ^^

phosit edited the test plan for this revision. (Show Details)Wed, Mar 27, 5:25 PM
phosit updated the Trac tickets for this revision.
phosit edited the test plan for this revision. (Show Details)Wed, Mar 27, 5:41 PM
sera added inline comments.Wed, Mar 27, 9:00 PM
source/rlinterface/RLInterface.h
167

Maybe keep the mg in the variable name? Also why a unique pointer instead of just using the destructor to clean up?

phosit added inline comments.Wed, Mar 27, 9:41 PM
source/rlinterface/RLInterface.h
167

Adding a "mg" feels redundant. Propably something like httpServerContext.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-ptr

sera added inline comments.Wed, Mar 27, 10:04 PM
source/rlinterface/RLInterface.h
167

Here the pointer is private and no copy can be obtained, therefore the Interface already serves as "unique pointer" or in other words is unmistakably the owner. Basically this feels like double wrapping.

phosit marked an inline comment as done.Thu, Mar 28, 6:30 PM
phosit added inline comments.
source/rlinterface/RLInterface.h
167

Externaly yes. But inside of RL::Interface member functions thous things are possible.

Stoping the context is the responsibility of the object. (If I would redesign mongoose in C++ you wouldn't have to manually stop the context.)

I'd like to use the destructor only for things where you need multiple member variables.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-ptr
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-dtor-ptr2

sera added inline comments.Sat, Mar 30, 3:08 PM
source/rlinterface/RLInterface.h
167

Well, guarding against internal misuses seems somewhat futile an undertaking in the first place, at least I find increasing size to achieve it questionable. Guess would be nice if one or two more people could voice there take.

phosit marked an inline comment as done.Sat, Mar 30, 6:37 PM
phosit added inline comments.
source/rlinterface/RLInterface.h
167

This patch is there to solve an internal misuse.
"Size increase" of what?

sera added inline comments.Sun, Mar 31, 5:20 PM
source/rlinterface/RLInterface.h
167

Source size obviously but that can be ignored for all intent and purpose. Binary size should increase while the worst case of size on the stack/heap with how you crafted it (empty deleter) should have been avoided.

Guess the binary size increase could be more than you expect.

phosit marked 2 inline comments as done.Mon, Apr 1, 5:03 PM
phosit added inline comments.
source/rlinterface/RLInterface.h
167

Since std::default_delete is an empty class implementations should optimize std::unique_ptr for empty deleter.
Binary size doesn't matter in parts where it's seldomly called (loaded in to cache).
To reduce source size i could use a function reference as deleter but that would increase stack usage.