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 the APT handler was removed after the response handler was called #1572

Merged

Conversation

joeljfischer
Copy link
Contributor

@joeljfischer joeljfischer commented Feb 26, 2020

Fixes #1559

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).

Unit Tests

Unit tests were run. New unit tests were investigated and added.

  • Test if the AddCommand handler is removed by the time the handler is called.

Core Tests

Tests were performed against the HU based on the reported bug.

  • Have a PAPT run from the handler of the PAPT to ensure that a new audio pass thru modal appears.

Core version / branch / commit hash / module tested against: Sync 3.0 (17
HMI name / version / branch / commit hash / module tested against: Sync 3.0

Summary

This PR changes the order of operations in the SDLResponseDispatcher to remove command / button / audio pass thru handlers before calling the response handler of the delete / unsubscribe.

Changelog

Bug Fixes
  • Fix audio pass thru handlers being deleted after the PAPT response handler is called, leading to situations where the APT handler would never be called if a new PAPT were started from the old PAPT response handler.

Tasks Remaining:

  • Fix unit tests

CLA

* Possible solution to second APT being cleared if it's sent in the PAPT response handler
…andler-called

# Conflicts:
#	SmartDeviceLink/SDLResponseDispatcher.m
@joeljfischer joeljfischer added bug A defect in the library manager-lifecycle Relating to the manager layer - lifecycle manager labels Feb 26, 2020
@joeljfischer joeljfischer self-assigned this Feb 26, 2020
@joeljfischer joeljfischer added this to In progress in v6.6.0 via automation Feb 26, 2020
@joeljfischer joeljfischer changed the title Fix the APT handler was removed after the response handler was called WIP: Fix the APT handler was removed after the response handler was called Feb 26, 2020
* Fix the handler was still called before the handler was removed (sometimes / race condition)
* Remove incorrect comments
* Add tests for checking the handler
@joeljfischer joeljfischer changed the title WIP: Fix the APT handler was removed after the response handler was called Fix the APT handler was removed after the response handler was called Feb 26, 2020
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some test cases are failing.

v6.6.0 automation moved this from In progress to Waiting for Review Feb 27, 2020
* All getters for the properties now use a dispatch_sync
* Now runs on the serial processing queue instead of the concurrent processing queue
* Reverted most of the tests to only check the properties serially since they're dispatch_sync'd now
v6.6.0 automation moved this from Waiting for Review to Reviewer approved Feb 28, 2020
@joeljfischer joeljfischer merged commit 963af93 into develop Feb 28, 2020
v6.6.0 automation moved this from Reviewer approved to Done Feb 28, 2020
@joeljfischer joeljfischer deleted the bugfix/issue-1559-fix-APT-removal-after-handler-called branch February 28, 2020 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect in the library manager-lifecycle Relating to the manager layer - lifecycle manager
Projects
No open projects
v6.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants