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 issue #6872 - Mutex lock has possibility to fail at runtime (returning status flag) #7423

Merged
merged 10 commits into from Sep 3, 2018

Conversation

Projects
None yet
@mprse
Member

mprse commented Jul 5, 2018

Description

This patch is a partial fix for issue #6872.

This is the first step which adds an assertion, so we get the indication in develop builds, not just debug.
Potentially in the next step we could perform total elimination of the error return in the non-time-limited Mutex claim.

More information can be found here:
https://jira.arm.com/browse/IOTMORF-2350

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[X] Breaking change
@kjbracey-arm

I had forgotten that trylock_for had made it in already.

Given that that is present, it might also make sense to add an overload for lock(void) (which is the one which ultimately will get a void return) and deprecate lock(millisec) in favour of trylock_for.

@@ -55,6 +55,9 @@ osStatus Mutex::lock(uint32_t millisec) {
if (osOK == status) {
_count++;
}
MBED_ASSERT(status == osErrorTimeout || status == osOK);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jul 5, 2018

Contributor

Unfortunately CMSIS-RTOS can also return "osErrorResource" if the mutex is locked and timeout is 0.

And there should be no error if the timeout specified was 0xffffffff. I think.

The maximally-tight assertion is I think:

MBED_ASSERT(status == osOK ||
            (status == osErrorResource && millisec == 0) ||
            (status == osErrorTimeout && millisec != osWaitForever));

Note that if adding an assert here, trylock_for can have its removed.

@cmonr cmonr added the needs: work label Jul 5, 2018

@mprse mprse force-pushed the mprse:mutex_lock_assert branch from 0afc717 to 880d1cc Jul 6, 2018

@mprse mprse changed the title from Add the assert to Mutex::lock - allow only osErrorTimeout and osOK. to Fix for issue #6872 - Mutex lock has possibility to fail at runtime (returning status flag) Jul 6, 2018

@mprse

This comment has been minimized.

Member

mprse commented Jul 6, 2018

@kjbracey-arm Thanks for the review. I followed your suggestion and added void Mutex::lock(void) member function and marked osStatus lock(uint32_t millisec=osWaitForever); as deprecated.
As a result of these operations the following additional changes were required:

  • remove default value from deprecated lock(uint32_t millisec=osWaitForever) member function since otherwise calling mutex.lock() would be ambiguous.
  • fix usage of Mutex::lock() function is source file - ignore returned value.
  • adapt tests-mbedmicro-rtos-mbed-mutex test - use new void lock(void) function instead of deprecated one.

The following questions appeared:

  • Which mbed version should be used in MBED_DEPRECATED_SINCE macro? I used mbed-os-5.10.0 ?
  • Now we have inconsistency between lock() and unlock(). Maybe the same modifications should be performed for Mutex::unlock() member function?
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 6, 2018

I was thinking it was premature to actually make the Mutex::lock() return void - suggesting the separation only to get the deprecation warning on the (millisecond) form - but if that few people really are checking the return value in-tree, maybe it's actually feasible. May actually lead to fewer problems overall, especially with binary compatibility, if that overload has void return from the beginning. I'll approve if others are fine with it.

Deprecated-since should say 5.10.

Yes, when Mutex::lock() is changed to void return, so should unlock be. And it should also assert either way.

@mprse

This comment has been minimized.

Member

mprse commented Jul 6, 2018

Changed definition of Mutex::unlock member function to void unlock(void); for consistency with void lock(void);.

@0xc0170 0xc0170 requested review from ARMmbed/mbed-os-core and bulislaw Jul 9, 2018

@cmonr cmonr added needs: review and removed needs: work labels Jul 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 13, 2018

[ ] Breaking change

Isn't this breaking change? I dont see any mention in the commit message but from the discussion and code, looks like it is. If yes, this will need careful consideration and review

@mprse

This comment has been minimized.

Member

mprse commented Jul 13, 2018

Isn't this breaking change? I dont see any mention in the commit message but from the discussion and code, looks like it is. If yes, this will need careful consideration and review

Marked this PR as "breaking change".

@mprse

This comment has been minimized.

Member

mprse commented Jul 20, 2018

@bulislaw

This comment has been minimized.

Member

bulislaw commented Aug 6, 2018

ping

stat = mutex.lock();
TEST_ASSERT_EQUAL(osOK, stat);
mutex.lock();

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 6, 2018

Contributor

We don;t return any status, but how do we verify if recursive lock here was successful or not?

This comment has been minimized.

@mprse

mprse Aug 7, 2018

Member

In the original version returned status has been checked. Since we do not have status now we can only execute two successive locks and two successive unlocks and show that the test case has been executed successfully. We do not have status, but please note that ASSERT has been added to the lock/unlock. In case of failure ASSERT will force the test to fail.

stat = mutex.unlock();
TEST_ASSERT_EQUAL(osOK, stat);
mutex.unlock();

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 6, 2018

Contributor

I am not sure how are we testing lock/unlock success/failure here? Should we modify the test to check from another thread if lock was applied or not and set success/fail based on that.

This comment has been minimized.

@mprse

mprse Aug 7, 2018

Member

ASSERT added in lock/unlock will force the test to fail in case of failure.
I don't think that we need here to use another thread to show that lock/unlock works since we got other test cases which proves that.

@deepikabhavnani

👍

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 7, 2018

Once this is rebased, we can start CI.

@mprse mprse force-pushed the mprse:mutex_lock_assert branch from efe7914 to 9d37ce9 Aug 8, 2018

@mprse

This comment has been minimized.

Member

mprse commented Aug 8, 2018

Once this is rebased, we can start CI.

Rebased and after consultation with @0xc0170 marked osStatus lock(uint32_t millisec) as deprecated since mbed-os-6.0.

@cmonr cmonr requested a review from ARMmbed/mbed-os-core Aug 8, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 8, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 8, 2018

@kjbracey-arm @deepikabhavnani @ARMmbed/mbed-os-core @bulislaw @0xc0170
Would anyone care to take a final look?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Aug 8, 2018

I think the deprecation of lock(millisec) is for now - 5.10. We know we want people to move to lock() or try_lock(millisec).

The main question is what the return type of lock() should be, which doesn't affect the above deprecation. If it returns void, which is what we ultimately want, it's a breaking change for anyone checking the return value without timeout.

But we can't deprecate just use of the return value. :( Either we do this now despite lack of deprecation or save it to some 6.0 point, where we'd be okay with a change without deprecation.

I don't want to sidestep by use of a lock2() or something, because we do ultimately want to get the point of having just void lock() to be a C++ Lockable.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 8, 2018

I think the deprecation of lock(millisec) is for now - 5.10. We know we want people to move to lock() or try_lock(millisec).

@mprse Please correct that

But we can't deprecate just use of the return value. :( Either we do this now despite lack of deprecation or save it to some 6.0 point, where we'd be okay with a change without deprecation.

I would vote the next major.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 2, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 2, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 2, 2018

/morph export-build

*/
osStatus lock(uint32_t millisec=osWaitForever);
MBED_DEPRECATED_SINCE("mbed-os-5.10.0", "Replaced with lock(), trylock() and trylock_for() functions")

This comment has been minimized.

@0xc0170

0xc0170 Sep 2, 2018

Member

Look at exporters failing (this line there, as mbed-os\rtos/Mutex.h(107): error: #79: expected a type specifier)

missing header file inclusion or what is this causing?

@NirSonnenschein

This comment has been minimized.

Contributor

NirSonnenschein commented Sep 2, 2018

@mprse please see multiple failures in export.

@mbed-ci

This comment has been minimized.

Address review comments.
Perform the following changes:
- change definition of `void Mutex::lock(void)` to `osStatus Mutex::lock(void)`.
- change definition of `void Mutex::unlock()` to `osStatus Mutex::unlock()`.
- use MBED_ERROR1 macro to check the lock/unlock operation status.
- add notes in the description of lock/unlock functions: "This function asserts status of the lock/unlock operation (will not return in case of failure). Use of the return value is deprecated, as the return is expected to become void in the future.".
- modify/add description of the return value.
- remove reference to Mbed 6.
- make `lock(millisec)` deprecated in favour of lock(), trylock() and trylock_for() functions.

@mprse mprse force-pushed the mprse:mutex_lock_assert branch from cc8c2a8 to fba9c4b Sep 3, 2018

@mprse

This comment has been minimized.

Member

mprse commented Sep 3, 2018

Looks like #include "platform/mbed_toolchain.h" is missing in Mutex.h.
Added missing include.

It was already added before (ac2db7c) and removed by mistake in the last commit while reverting wrong changes.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 3, 2018

/morph build

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 3, 2018

Looks good.

@adbridge adbridge merged commit c2fdc0d into ARMmbed:master Sep 3, 2018

14 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.05%) RAM(+0.0%)
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_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 597 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9904 cycles (-958 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

@0xc0170 0xc0170 removed the needs: CI label Sep 3, 2018

@MarceloSalazar

This comment has been minimized.

Contributor

MarceloSalazar commented Sep 21, 2018

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