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

Race condition happens in HttpRequest if you modify reference count in two threads #19169

Open
tanwenzhao opened this Issue Nov 14, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@tanwenzhao
Copy link

tanwenzhao commented Nov 14, 2018

  • cocos2d-x version: 3.* and 2.*
    This is a serious issue and exists in both 2.* and 3.,
    Code below is a typical code of HttpRequest in version 2.
    , there is race condition btweeen "request->release()" and reference count changes to request in networkThread funciton at HttpClient.cpp. This will cause request has wrong reference count and sometimes be freed and used later, causing crash and random behavior. If you use userdata in request, passing in function pointers or write to buffer, this will cause crash.

In version 3.*, you still change request's reference count in networkThread when dequing the request from s_requestQueue. So there is also possible race conditions if the upper caller code is similar to below with request->release()

Our temporary fix is to remove both the request->release() in main thread and the retain in send().
Race condition happens in HttpRequest if you modify reference count in two threads. This is a general potential problem and need to pay lots of attention.

HttpRequest* request = new HttpRequest();
request->setRequestType(HttpRequest::Type::GET);
request->setUrl("http://YOUR_URL");
request->setResponseCallback(CC_CALLBACK_2(HttpClientTest::onHttpRequestCompleted, this));
request->setTag("Get test");
HttpClient::getInstance()->send(request);
request->release();

@zcgit

This comment has been minimized.

Copy link
Contributor

zcgit commented Nov 18, 2018

yes, you should use HttpClient::getInstance()->sendImmediate(req), hope this is useful for you :)

@tanwenzhao

This comment has been minimized.

Copy link

tanwenzhao commented Nov 18, 2018

you are right. I briefly took a look of sendImmediate, it does not change request's reference count in worker thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment