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

*Fix: GLThread and other threads access sDownloaderMap in a thread-un… #16326

Merged
merged 1 commit into from
Mar 7, 2017

Conversation

pwang08
Copy link
Contributor

@pwang08 pwang08 commented Aug 5, 2016

GLThread and other threads(js_load_remote_image creates CCDownloader in another thread) access sDownloaderMap in a thread-unsafe way, which will result in crash in some occasion on Android.

…safe way, which will result in crash in some occasion on Android.
@dumganhar
Copy link

dumganhar commented Aug 12, 2016

Hi, pwang08,
I notice that Downloader instance is usually constructed in GL thread.
And the callback functions are also invoked from GL thread. Could this cause thread problems?

Cocos2dxHelper.runOnGLThread(new Runnable() {
            @Override
            public void run() {
                nativeOnProgress(_id, id, downloadBytes, downloadNow, downloadTotal);
            }
        });

@pwang08
Copy link
Contributor Author

pwang08 commented Aug 12, 2016

void __JSDownloaderDelegator::downloadAsync() { retain(); auto t = std::thread(&__JSDownloaderDelegator::startDownload, this); t.detach(); }

As the code above shows: __JSDownloaderDelegator::startDownload is called in another thread, in which a Downloader instance is created. As the code below shows:

void __JSDownloaderDelegator::startDownload() { if (auto texture = Director::getInstance()->getTextureCache()->getTextureForKey(_url)) { onSuccess(texture); } else { _downloader = std::make_shared<cocos2d::network::Downloader>(); } }

@dumganhar
Copy link

dumganhar commented Aug 12, 2016

@pwang08 , it makes sense.

@pandamicro , Could you please check it since if I remember correctly, __JSDownloaderDelegator stuff was written by u.

@pandamicro
Copy link
Contributor

pandamicro commented Aug 15, 2016

Yes, it was originally created by me, at the time, there was a performFunctionInCocosThread to ensure everything is thread safe. But @yangws have refactored the network module and removed these code because network it self will make sure callbacks are invoked in cocos thread.

@minggo minggo closed this Mar 7, 2017
@minggo minggo reopened this Mar 7, 2017
@minggo
Copy link
Contributor

minggo commented Mar 7, 2017

just close/reopen it to trigger ci. This PR seems good.

@minggo minggo added this to the 3.15 milestone Mar 7, 2017
@minggo minggo merged commit 0cde5c6 into cocos2d:v3 Mar 7, 2017
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Mar 9, 2017
…futurePRs

* commit '5c0c6d2e1e4b1772efc239d23f8cdaf8ce79a898':
  More powerful Uri class, adds unit test for Uri class. Refactors some code in SocketIO & Websocket. (cocos2d#17472)
  Prevent signed/unsigned mismatch (cocos2d#17468)
  fix the issue that onEnterTransitionDidFinish() is not invoked at first scene change (cocos2d#17466)
  csbload error bug (cocos2d#17465)
  *Fix: GLThread and other threads access sDownloaderMap in a thread-unsafe way, which will result in crash in some occasion on Android. (cocos2d#16326)
  fixed cocos2d#17416: Wrong default port of SocketIO connection. (cocos2d#17459)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17458)
  fixed cocos2d#17427: lws_parse_url has wrong behavior, it parses ’ws://domain.com/abc/d’ to ‘path: abc/d’ rather than ‘path: /adb/d’. (cocos2d#17455)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17456)
  Prevent unexpected calls to unscheduled selector in long updates. (cocos2d#17431)
  fix the broken of lua-tests after PR cocos2d#17445 was merged. (cocos2d#17454)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17453)
  Solve the error logic in spine runtime. (cocos2d#17448)
  cocos2d::Sequence::isDone() checks that the last action is actually done. (cocos2d#17437)
  Websocket bug fix after PR#17440 was merged. (cocos2d#17450)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#17449)
  Some fixes for search paths: (cocos2d#17435)
  update external version (cocos2d#17446)
  fixed cocos2d#17433: [webSocket ] webSocket random crash in win32 (cocos2d#17440)
  Fix some local variable names in tests (cocos2d#17445)

# Conflicts:
#	web
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

Successfully merging this pull request may close these issues.

None yet

4 participants