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

Added missing _mutex->unlock() to FileBase::lookup(). #8451

Merged
merged 1 commit into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@korjaa
Contributor

korjaa commented Oct 17, 2018

Description

My modified Pelion Device Management Client has been getting stuck in the startup for a while now. After days of debugging, I finally was able to pinpoint the system to a mutex deadlock.

I don't thought understand how this has worked with the already released PDMC.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@kjbracey-arm, related to the discussion on the hallway, this fix is related to previous modification done in here #7924

@korjaa

This comment has been minimized.

Contributor

korjaa commented Oct 17, 2018

I understood the ScopedLock would be a better way to fix this. If we go with the better fix, please feel free to close this PR.

@kjbracey-arm

kjbracey-arm approved these changes Oct 17, 2018 edited

I do recall seeing this error during development of #7924.

I corrected it by changing to ScopedLock<PlatformMutex>, but when that was rejected during review, I dropped the commit, forgetting that the commit was actually fixing this mutex bug, not just a style adjustment. Whoops.

You could switch to ScopedLock again, but this is okay.

I think we've not seen a problem because it's a recursive mutex, so wouldn't show any issues until you've got two threads doing file opening (with at least one using /default.)

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 17, 2018

Actually, the ScopedLock version is a bit ugly until #8001, and that was also deferred from #7924, so won't be in until 5.11. This one is fine.

@korjaa

This comment has been minimized.

Contributor

korjaa commented Oct 17, 2018

👍

@cmonr cmonr added the needs: CI label Oct 17, 2018

@cmonr

cmonr approved these changes Oct 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 17, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 17, 2018

IAR network license issues.
/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 17, 2018

Build : SUCCESS

Build number : 3387
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8451/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Oct 17, 2018

@cmonr cmonr merged commit 1a6d2f6 into ARMmbed:master Oct 17, 2018

15 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Passed, 667 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9855 cycles (+749 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Oct 17, 2018

@korjaa korjaa deleted the korjaa:fix_possible_filebase_deadlock branch Oct 18, 2018

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