heroiclabs / nakama-cpp

Generic C/C++ client for Nakama server.
https://heroiclabs.com/docs/cpp-client-guide
Apache License 2.0
69 stars 25 forks source link

Callback from authentication request results in undefined behavior if Nakama client has been destroyed: #26

Closed crowder closed 4 years ago

crowder commented 4 years ago

After a call to authenticateDevice(), if the endpoint being connected to is slow (in my case, firewalled due to a security group configuration "bug"), and the NClient object is disconnect()ed and destroyed, I am still seeing what looks like a callback from within nakama-cpp.dll (I'm using the Unreal client, but have looked through the code enough to suspect the bug is here) will still fire. I cannot figure out how to cancel this outstanding auth request in a clean way, and I do not see code in this repo that does so either.

Thanks in advance for your time.

novabyte commented 4 years ago

Thanks for the report @crowder. @Dimon4eg Please can you take a look

Dimon4eg commented 4 years ago

disconnect does nothing:

void RestClient::disconnect()
{
}

We delete all requests in destructor and print warning:

RestClient::~RestClient()
{
    disconnect();

    if (_reqContexts.size() > 0)
    {
        NLOG(NLogLevel::Warn, "Not handled %u request(s) detected.", _reqContexts.size());

        for (RestReqContext* reqContext : _reqContexts)
        {
            delete reqContext;
        }

        _reqContexts.clear();
    }
}

So, it's not possible to receive callback after NClient is destroyed. @crowder Please show me call stack when you receive callback and logs.

crowder commented 4 years ago

I'll have to build a debug version of the dll to give you a stack, but I will do so at my earliest convenience. Why is it that disconnect does not destroy request contexts? That seems... unexpected?

Dimon4eg commented 4 years ago

At first I did not implement disconnect because cpprest library doesn't have such functionality. Now I implemented that by destroying cpprest client. Changes are in disconnect branch. Can you try that?

crowder commented 4 years ago

Okay. I had to hack a fix into the websocketpp library code to get a build that was usable, and there's a dependency on yasm not mentioned by the build-from-source docs for Nakama-cpp. Also, the disconnect branch does not seem to fix my bug, BUT, I was finally able to generate a build of the DLL with debug symbols, so here's my stack:

>   msvcp140d.dll!mtx_do_lock(_Mtx_internal_imp_t * mtx, const xtime * target) Line 102 C++
    msvcp140d.dll!_Mtx_lock(_Mtx_internal_imp_t * mtx) Line 176 C++
    nakama-cpp.dll!std::_Mutex_base::lock() Line 51 C++
    nakama-cpp.dll!std::lock_guard<std::mutex>::lock_guard<std::mutex>(std::mutex & _Mtx) Line 437  C++
    nakama-cpp.dll!Nakama::NHttpClientCppRest::finishReq(unsigned __int64 id, std::shared_ptr<Nakama::NHttpResponse> response) Line 176 C++
    nakama-cpp.dll!Nakama::NHttpClientCppRest::finishReqWithError(unsigned __int64 id, int statusCode, std::string && reason) Line 197  C++
    nakama-cpp.dll!`void <lambda>(Concurrency::task<web::http::http_response>)'::`1'::catch$9() Line 136    C++
    [External Code] 
    nakama-cpp.dll!Nakama::NHttpClientCppRest::request::__l2::<lambda>(Concurrency::task<web::http::http_response> previousTask) Line 112   C++
    [External Code] 
    [Async Call]    
    nakama-cpp.dll!Nakama::NHttpClientCppRest::request(const Nakama::NHttpRequest & req, const std::function<void __cdecl(std::shared_ptr<Nakama::NHttpResponse>)> & callback) Line 108 C++
    nakama-cpp.dll!Nakama::RestClient::sendReq(Nakama::RestReqContext * ctx, Nakama::NHttpReqMethod method, std::string && path, std::string && body, std::multimap<std::string,std::string,std::less<std::string>,std::allocator<std::pair<std::string const ,std::string>>> && args) Line 160 C++
    nakama-cpp.dll!Nakama::RestClient::authenticateDevice(const std::string & id, const nonstd::optional_lite::optional<std::string> & username, const nonstd::optional_lite::optional<bool> & create, const std::map<std::string,std::string,std::less<std::string>,std::allocator<std::pair<std::string const ,std::string>>> & vars, std::function<void __cdecl(std::shared_ptr<Nakama::NSessionInterface>)> successCallback, std::function<void __cdecl(Nakama::NError const &)> errorCallback) Line 337    C++
    nakama-cpp.dll!NClient_authenticateDevice(NClient_ * client, const char * id, const char * username, bool create, NStringMap_ * vars, void * reqData, void(*)(NClient_ *, void *, NSession_ *) successCallback, void(*)(NClient_ *, void *, const NError *) errorCallback) Line 161 C++
    UE4Editor-WalkAndTalk.dll!NakamaWrapper::NClientWrapper::authenticateDevice(const std::string & id, const nonstd::optional_lite::optional<std::string> & username, const nonstd::optional_lite::optional<bool> & create, const std::map<std::string,std::string,std::less<std::string>,std::allocator<std::pair<std::string const ,std::string>>> & vars, std::function<void __cdecl(std::shared_ptr<NakamaWrapper::NSessionInterface>)> successCallback, std::function<void __cdecl(NakamaWrapper::NError const &)> errorCallback) Line 211    C++
    UE4Editor-WalkAndTalk.dll!FNakamaNetworkManager::Start() Line 63    C++
    UE4Editor-WalkAndTalk.dll!UWalkAndTalkGameInstance::Init() Line 45  C++
    UE4Editor-Engine.dll!UGameInstance::InitializeForPlayInEditor(int PIEInstanceIndex, const FGameInstancePIEParameters & Params) Line 277 C++
    UE4Editor-UnrealEd.dll!UEditorEngine::CreatePIEGameInstance(int InPIEInstance, bool bInSimulateInEditor, bool bAnyBlueprintErrors, bool bStartInSpectatorMode, bool bRunAsDedicated, bool bPlayStereoscopic, float PIEStartTime) Line 3098  C++
    UE4Editor-UnrealEd.dll!UEditorEngine::PlayInEditor(UWorld * InWorld, bool bInSimulateInEditor, FPlayInEditorOverrides Overrides) Line 2589  C++
Dimon4eg commented 4 years ago

ugh, finally understood reason of crash and fixed it. Please check latest disconnect branch.

Dimon4eg commented 4 years ago

btw what did you fixed in the websocketpp?

crowder commented 4 years ago

I ran into https://github.com/zaphoyd/websocketpp/issues/794 ... I threw up a diff hunk that shows what I changed. Can't tell if I ran into it because of the VS2019 compiler or what, didn't spend a ton of time on it, either, so it's possibly wildly broken.

crowder commented 4 years ago

Looks like the latest in the disconnect branch does indeed resolve my crash! Thanks a ton!

Dimon4eg commented 4 years ago

Cool! Thanks for reporting the error 👍