Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
crowder opened this issue Feb 21, 2020 · 10 comments
Assignees

Comments

@crowder
Copy link

crowder commented Feb 21, 2020

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
Copy link
Member

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

@Dimon4eg
Copy link
Contributor

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
Copy link
Author

crowder commented Feb 24, 2020

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
Copy link
Contributor

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
Copy link
Author

crowder commented Feb 28, 2020

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
Copy link
Contributor

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

@Dimon4eg
Copy link
Contributor

btw what did you fixed in the websocketpp?

@crowder
Copy link
Author

crowder commented Feb 28, 2020

I ran into zaphoyd/websocketpp#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
Copy link
Author

crowder commented Feb 28, 2020

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

@Dimon4eg
Copy link
Contributor

Cool! Thanks for reporting the error 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants