ZipUtils thread-safe patch for #1562 #1933

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@xeroo

xeroo commented Jan 30, 2013

Sorry for deleting #1924 by mistake.

Access to files from APK is improved in #1562. But modifying m_data in ZipUtils::getFileData has a thread-safety issue. This patch fixes it with a simple lock.

There maybe a better way. Please review.

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 1, 2013

Contributor

Why it has thread-safe issue?

Contributor

minggo commented Feb 1, 2013

Why it has thread-safe issue?

@xeroo

This comment has been minimized.

Show comment Hide comment
@xeroo

xeroo Feb 1, 2013

CCFileUtils::getFileData uses s_pZipFile->getFileData to get data from apk, where s_pZipFile is a shared object. And m_data in s_pZipFile is used and modified inside s_pZipFile->getFileData which is also a shared object. The following lines from s_pZipFile->getFileData are exactly the point where thread-safe issue will happen.

    int nRet = unzGoToFilePos(m_data->zipFile, &fileInfo.pos);
    CC_BREAK_IF(UNZ_OK != nRet);

    nRet = unzOpenCurrentFile(m_data->zipFile);
    CC_BREAK_IF(UNZ_OK != nRet);

    pBuffer = new unsigned char[fileInfo.uncompressed_size];
    int nSize = 0;
    nSize = unzReadCurrentFile(m_data->zipFile, pBuffer, fileInfo.uncompressed_size);

xeroo commented Feb 1, 2013

CCFileUtils::getFileData uses s_pZipFile->getFileData to get data from apk, where s_pZipFile is a shared object. And m_data in s_pZipFile is used and modified inside s_pZipFile->getFileData which is also a shared object. The following lines from s_pZipFile->getFileData are exactly the point where thread-safe issue will happen.

    int nRet = unzGoToFilePos(m_data->zipFile, &fileInfo.pos);
    CC_BREAK_IF(UNZ_OK != nRet);

    nRet = unzOpenCurrentFile(m_data->zipFile);
    CC_BREAK_IF(UNZ_OK != nRet);

    pBuffer = new unsigned char[fileInfo.uncompressed_size];
    int nSize = 0;
    nSize = unzReadCurrentFile(m_data->zipFile, pBuffer, fileInfo.uncompressed_size);
@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 1, 2013

Contributor

But cocos2d-x is not thread-safe.
It means that you can not invoke CCFileUtils::getFileData in more than one thread.

Contributor

minggo commented Feb 1, 2013

But cocos2d-x is not thread-safe.
It means that you can not invoke CCFileUtils::getFileData in more than one thread.

@xeroo

This comment has been minimized.

Show comment Hide comment
@xeroo

xeroo Feb 1, 2013

CCTextureCache::addImageAsync seems to be the standalone thread that uses CCFileUtils::getFileData. May be thread-safe should be guaranteed in CCImage::initWithImageFileThreadSafe, but it isn't.

xeroo commented Feb 1, 2013

CCTextureCache::addImageAsync seems to be the standalone thread that uses CCFileUtils::getFileData. May be thread-safe should be guaranteed in CCImage::initWithImageFileThreadSafe, but it isn't.

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 1, 2013

Contributor

Yep, you are right.
So i think we should write thread-safe codes in CCImage::initWithImageFileThreadSafe, but not ZipUtils.

What's your opinion?

Contributor

minggo commented Feb 1, 2013

Yep, you are right.
So i think we should write thread-safe codes in CCImage::initWithImageFileThreadSafe, but not ZipUtils.

What's your opinion?

@xeroo

This comment has been minimized.

Show comment Hide comment
@xeroo

xeroo Feb 1, 2013

Sure, that seems the best solution. So, add a CCFileUtils::getFileDataThreadSafe function? Or, let CCFileUtils::getFileData use shared apk structure cache but separated offset data?

Maybe we should also ask @mingulov for his opinion?

xeroo commented Feb 1, 2013

Sure, that seems the best solution. So, add a CCFileUtils::getFileDataThreadSafe function? Or, let CCFileUtils::getFileData use shared apk structure cache but separated offset data?

Maybe we should also ask @mingulov for his opinion?

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 1, 2013

Contributor

Or, let CCFileUtils::getFileData use shared apk structure cache but separated offset data?

How to do it?

Contributor

minggo commented Feb 1, 2013

Or, let CCFileUtils::getFileData use shared apk structure cache but separated offset data?

How to do it?

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 1, 2013

Contributor

After detail reading the codes of ZipFile::getFileData(), i found there were two things may cause thread-safe problem

  1. the operation of m_data->fileList
    fileList is a map, and there are only read operations on it, so it is thread-safe.
  2. unzXXX functions
    these functions are all c functions, and there is not global variables and static variables in functions, so it is thread-safe.

All in a word, ZipFile::getFileData() is thread-safe.
Right?

Contributor

minggo commented Feb 1, 2013

After detail reading the codes of ZipFile::getFileData(), i found there were two things may cause thread-safe problem

  1. the operation of m_data->fileList
    fileList is a map, and there are only read operations on it, so it is thread-safe.
  2. unzXXX functions
    these functions are all c functions, and there is not global variables and static variables in functions, so it is thread-safe.

All in a word, ZipFile::getFileData() is thread-safe.
Right?

@mingulov

This comment has been minimized.

Show comment Hide comment
@mingulov

mingulov Feb 1, 2013

Contributor

No, for example unzGoToFilePos64 (called from unzGoToFilePos) is not thread-safe - it changes the provided 1st parameter - unzFile (m_data->zipFile - which is quite complex structure ) - so if ZipFile::getFileData will be called from 2 or more threads simultaneously - the behaviour is undefined.
So I think such mutexes might be useful.

But anyway, I think there is a problem with CCImage::initWithImageFileThreadSafe - as it really not a thread safe. The only difference with usual CCImage::initWithImageFile - is a missing call to fullPathForFilename(), but it is called at almost every CCFileUtils::getFileData - and CCFileUtils usually does not support different threads.

Contributor

mingulov commented Feb 1, 2013

No, for example unzGoToFilePos64 (called from unzGoToFilePos) is not thread-safe - it changes the provided 1st parameter - unzFile (m_data->zipFile - which is quite complex structure ) - so if ZipFile::getFileData will be called from 2 or more threads simultaneously - the behaviour is undefined.
So I think such mutexes might be useful.

But anyway, I think there is a problem with CCImage::initWithImageFileThreadSafe - as it really not a thread safe. The only difference with usual CCImage::initWithImageFile - is a missing call to fullPathForFilename(), but it is called at almost every CCFileUtils::getFileData - and CCFileUtils usually does not support different threads.

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo Feb 2, 2013

Contributor

@mingulov
Yep, you are right.

The key problem is because m_data is a shared object. But i don't think it is a good idea to add mutex in the function. Because it will slow down the speed even it is not needed.

The codes that will cause thread-safe issue is CCImage::initWithImageFileThreadSafe, so i think it is better only modify its codes, maybe not invoke getFileData.

What's your opinion?

Contributor

minggo commented Feb 2, 2013

@mingulov
Yep, you are right.

The key problem is because m_data is a shared object. But i don't think it is a good idea to add mutex in the function. Because it will slow down the speed even it is not needed.

The codes that will cause thread-safe issue is CCImage::initWithImageFileThreadSafe, so i think it is better only modify its codes, maybe not invoke getFileData.

What's your opinion?

@minggo

This comment has been minimized.

Show comment Hide comment
@minggo

minggo May 7, 2013

Contributor

I have to close this PR, because it is not the best solution.
I have created an issue to track this bug.

Thank you for reporting this bug.

Contributor

minggo commented May 7, 2013

I have to close this PR, because it is not the best solution.
I have created an issue to track this bug.

Thank you for reporting this bug.

@minggo minggo closed this May 7, 2013

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