Fix/ccb 3141 change command request inheritance structure #3591

VladSemenyuk · 2020-12-07T12:54:48Z

Fixes #3141, #2935
This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Unit tests, ATF

Summary

Changed commands requests inheritance structure.
Fixed data races in request controller.

Enhancements

CommonRequestImpl class was reworked in order to keep only common
requests logic. Created new class - RequestFromMobile for logic specific for mobile
requests only.
Reworked logic of handling requests with multiple interfaces.
Used common methods for checks of pending requests.
Functions OnUpdateTimeOut and HahdleOnEvent added to CommandRequestImpl for correct synchorization and processing of current request state, that helps to avoid data races.
Current states processing:

Bug Fixes

Fix data races in request controller

There was a problem that request controller might
destroy the command which is currently executed by
another thread in case of application disconnect.
When app gets disconnected, SDL destroys all pending
and waiting for response requests belongs to
unregistered application. Data race is happening
when request controller destroys the command which
is currently handles response. To resolve this data
race the following changes were done:

terminateWaitingForResponseAppRequests function
was updated to iterate over all pending requests and
consider possibility to terminate it via CleanUp()
call. Requests will be terminated only in case of
successful cleanup
During cleanup, mobile requests unsubscribes from
all events to avoid getting kProcessEvent state
Cleanup is considered as successful in case if
command is in kAwaiting state
In case the command in:
kAwaitingResponse - command will be dropped by
request controller as usual as there no data races
kTimedOut - command is executing OnTimeout() call.
Request controller will keep this command in
waiting_for_response_ list as it will be dropped
later by another thread after OnTimeout() execution
kProcessEvent - command is executing on_event() call.
Request controller will keep this command in
waiting_for_response_ list as it will be dropped
later by another thread after on_event() execution

Prevent command destruction after on_event

Was added workaround to prevent command destruction
after on_event() call because some additional acitons
should be done after that. Workaround is to retain
requests shared pointer instance before on_event()
and release it after on_event() and state check.
This allows to remove another workaround logic and
provides better control on a command lifetime from
the outside.

Tasks Remaining:

[Task 1]
[Task 2]

CLA

I have signed the CLA

Fix data races during requests event processing.

VladSemenyuk · 2020-12-07T13:19:00Z

@JackLivio this PR is ready for review

…itance_structure

jacobkeeler · 2021-01-15T15:33:15Z

Unfortunately since this PR includes major API changes (due to changes in the inheritance structure of several classes), we will likely need to wait for a major release to include these changes.

jacobkeeler · 2021-01-15T16:36:08Z

As a note, we could include this by changing the coming April release to a major release for SDL Core, and this shouldn't affect any other releases so long as we don't include any major API changes to the RPC Spec.

jordynmackool · 2021-03-05T17:09:38Z

The Steering Committee voted on 2021-01-19 to not include PR #3591 resolving the prioritized Core issue #3141 in the April 14, 2021 releases.

jordynmackool · 2021-07-20T14:19:57Z

@VladSemenyuk this PR has conflicts that need to be resolved. Can you please resolve the conflict and then tag me when this PR is ready for Livio's review. Thanks!

AKalinich-Luxoft · 2021-07-20T16:07:41Z

@VladSemenyuk this PR has conflicts that need to be resolved. Can you please resolve the conflict and then tag me when this PR is ready for Livio's review. Thanks!

@jordynmackool we have plans to resolve conflicts in this PR after merge of #3726 which is currently on Livio review because this PR has a lot of intersections with the current fix which will cause new conflicts. Please let me know if you have plans to merge this fix before the mentioned feature delivery.

jordynmackool · 2021-07-22T16:11:13Z

@AKalinich-Luxoft thanks for the update. Livio is aligned with your plan to wait for the merge of #3726 to then resolve the noted conflicts on this PR. Please tag me once this is ready for our review. Thanks!

Jack-Byrne · 2021-07-27T14:28:25Z

@AKalinich-Luxoft #3726 has been approved so the changes for this pr can be rebased now.

AKalinich-Luxoft · 2021-07-27T14:31:15Z

@AKalinich-Luxoft #3726 has been approved so the changes for this pr can be rebased now.

@JackLivio is it possible to merge #3726 to develop? The current fix is targeted to develop so it's better to rebase against this branch containing 3726

Jack-Byrne · 2021-07-29T15:24:26Z

@AKalinich-Luxoft I believe #3726 review is still in progress.

theresalech · 2021-08-11T17:20:12Z

Hi @VladSemenyuk , #3726 has now been merged into develop. Can you please rebase this PR, resolve merge conflicts, and let us know when the PR is ready for Livio's review? Thank you!

AKalinich-Luxoft · 2021-08-17T16:23:59Z

Hi @VladSemenyuk , #3726 has now been merged into develop. Can you please rebase this PR, resolve merge conflicts, and let us know when the PR is ready for Livio's review? Thank you!

@theresalech Luxoft is in progress with resolving all the conflicts and do our best to get this fixed on time

jordynmackool · 2021-08-20T14:19:34Z

Hi @AKalinich-Luxoft, Livio is going to begin Release candidate prep, starting on 2021-08-30. We will need to make progress on this PR asap for it to be included in the upcoming release. Can you please provide timing on when Luxoft will have this PR ready for Livio review? Thanks!

…itance_structure

Jack-Byrne · 2021-08-26T14:58:52Z

@VladSemenyuk Hi, is this ready for review?

AKalinich-Luxoft · 2021-08-26T15:03:37Z

@VladSemenyuk Hi, is this ready for review?

@JackLivio the major part of this fix is ready for Livio review and all merge conflicts have been resolved. Notice that some minor fixup commits are expected here to fix a regression found after merging the latest changes from the develop branch.

Jack-Byrne · 2021-08-26T20:04:16Z

@AKalinich-Luxoft Just wanted to note I am seeing failures in the test set for OnResetTimeout

test_scripts/API/Restructuring_OnResetTimeout/003_BC_OnResetTimeout_GENERIC_ERROR_in_case_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/004_BC_OnResetTimeout_HMI_did_not_respond_notification_without_resetPeriod.lua
test_scripts/API/Restructuring_OnResetTimeout/006_BC_OnResetTimeout_with_wrong_methodName_and_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/007_BC_OnResetTimeout_with_wrong_requestID_and_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/015_BC_OnResetTimeout_ResetPeriod_is_zero.lua
test_scripts/API/Restructuring_OnResetTimeout/029_BC_OnResetTimeout_No_Reset_GENERIC_ERROR_non_zero_DefaultTimeoutCompensation.lua
test_scripts/API/Restructuring_OnResetTimeout/030_BC_OnResetTimeout_Reset_GENERIC_ERROR_non_zero_DefaultTimeoutCompensation.lua

That gives to event dispatcher some time to figure out that event should not be rasied for a finalized request and this request will be released soon. Also, this allows to avoid calling some virtual functions on pointer to invalidated object.

Now reference counter is increased indirectly by event dispatcher before calling `HandleOnEvent()` of specific observer. This will guarantee that during `HandleOnEvent` the command object is still be alive.

AKalinich-Luxoft · 2021-08-27T01:25:46Z

@AKalinich-Luxoft Just wanted to note I am seeing failures in the test set for OnResetTimeout

test_scripts/API/Restructuring_OnResetTimeout/003_BC_OnResetTimeout_GENERIC_ERROR_in_case_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/004_BC_OnResetTimeout_HMI_did_not_respond_notification_without_resetPeriod.lua
test_scripts/API/Restructuring_OnResetTimeout/006_BC_OnResetTimeout_with_wrong_methodName_and_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/007_BC_OnResetTimeout_with_wrong_requestID_and_HMI_did_not_respond.lua
test_scripts/API/Restructuring_OnResetTimeout/015_BC_OnResetTimeout_ResetPeriod_is_zero.lua
test_scripts/API/Restructuring_OnResetTimeout/029_BC_OnResetTimeout_No_Reset_GENERIC_ERROR_non_zero_DefaultTimeoutCompensation.lua
test_scripts/API/Restructuring_OnResetTimeout/030_BC_OnResetTimeout_Reset_GENERIC_ERROR_non_zero_DefaultTimeoutCompensation.lua

@JackLivio thank you for mentioning this. All these scripts have been fixed in the latest commits and are now PASSED.

AKalinich-Luxoft · 2021-08-27T01:28:32Z

@jordynmackool @JackLivio please notice that few more commits were pushed to this PR and now all regression tests are passed on our side which means that the current version of the fix has been finalized and no more changes are planned. You can proceed with the review of the entire fix now.

VladSemenyuk added 4 commits November 25, 2020 14:04

Change command request inheritance structure.

6393ac2

Fix data races during requests event processing.

Change commands according to new inheritance structure.

7234d7d

Change unit tests according new inheritance structure.

ecb2c7f

Fix review comments

c8049e2

VladSemenyuk added 2 commits January 14, 2021 14:05

Merge branch 'develop' into fix/CCB_3141_Change_command_request_inher…

0748e2a

…itance_structure

Fix build after merge

7a35494

VladSemenyuk mentioned this pull request Jan 26, 2021

CCB: Crash related to HandleOnEvent #3410

Closed

jordynmackool added fix submitted and removed fix submitted labels Jul 20, 2021

v-seme12 and others added 3 commits August 25, 2021 13:44

Merge branch 'develop' into fix/CCB_3141_Change_command_request_inher…

278b194

…itance_structure

Fix format style

931dbfe

Merge branch 'develop' into fix/CCB_3141_Change_command_request_inher…

88d0354

…itance_structure

Fix recursion in SetGlobalProperties

c9c9faa

Add delay before request releasing

6843927

That gives to event dispatcher some time to figure out that event should not be rasied for a finalized request and this request will be released soon. Also, this allows to avoid calling some virtual functions on pointer to invalidated object.

AKalinich-Luxoft added 2 commits August 26, 2021 21:24

Fix data races in EventDispatcher

86d4155

Now reference counter is increased indirectly by event dispatcher before calling `HandleOnEvent()` of specific observer. This will guarantee that during `HandleOnEvent` the command object is still be alive.

Revert changes for SetGlobalProperties

2629c81

Jack-Byrne approved these changes Aug 27, 2021

View reviewed changes

Jack-Byrne merged commit b9a2a53 into smartdevicelink:develop Aug 27, 2021

This was referenced Aug 27, 2021

CCB: Change command request inheritance structure #3141

Closed

SDL destroys currently running mobile app requests on app unregistration #2935

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/ccb 3141 change command request inheritance structure #3591

Fix/ccb 3141 change command request inheritance structure #3591

VladSemenyuk commented Dec 7, 2020 •

edited

Loading

VladSemenyuk commented Dec 7, 2020 •

edited

Loading

jacobkeeler commented Jan 15, 2021

jacobkeeler commented Jan 15, 2021

jordynmackool commented Mar 5, 2021 •

edited

Loading

jordynmackool commented Jul 20, 2021

AKalinich-Luxoft commented Jul 20, 2021

jordynmackool commented Jul 22, 2021

Jack-Byrne commented Jul 27, 2021

AKalinich-Luxoft commented Jul 27, 2021 •

edited

Loading

Jack-Byrne commented Jul 29, 2021

theresalech commented Aug 11, 2021

AKalinich-Luxoft commented Aug 17, 2021

jordynmackool commented Aug 20, 2021

Jack-Byrne commented Aug 26, 2021

AKalinich-Luxoft commented Aug 26, 2021

Jack-Byrne commented Aug 26, 2021

AKalinich-Luxoft commented Aug 27, 2021

AKalinich-Luxoft commented Aug 27, 2021

Fix/ccb 3141 change command request inheritance structure #3591

Fix/ccb 3141 change command request inheritance structure #3591

Conversation

VladSemenyuk commented Dec 7, 2020 • edited Loading

Risk

Testing Plan

Summary

Enhancements

Bug Fixes

Tasks Remaining:

CLA

VladSemenyuk commented Dec 7, 2020 • edited Loading

jacobkeeler commented Jan 15, 2021

jacobkeeler commented Jan 15, 2021

jordynmackool commented Mar 5, 2021 • edited Loading

jordynmackool commented Jul 20, 2021

AKalinich-Luxoft commented Jul 20, 2021

jordynmackool commented Jul 22, 2021

Jack-Byrne commented Jul 27, 2021

AKalinich-Luxoft commented Jul 27, 2021 • edited Loading

Jack-Byrne commented Jul 29, 2021

theresalech commented Aug 11, 2021

AKalinich-Luxoft commented Aug 17, 2021

jordynmackool commented Aug 20, 2021

Jack-Byrne commented Aug 26, 2021

AKalinich-Luxoft commented Aug 26, 2021

Jack-Byrne commented Aug 26, 2021

AKalinich-Luxoft commented Aug 27, 2021

AKalinich-Luxoft commented Aug 27, 2021

VladSemenyuk commented Dec 7, 2020 •

edited

Loading

VladSemenyuk commented Dec 7, 2020 •

edited

Loading

jordynmackool commented Mar 5, 2021 •

edited

Loading

AKalinich-Luxoft commented Jul 27, 2021 •

edited

Loading