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

Make ChoiceSetManager operations blocking #1155

Merged

Conversation

bilal-alsharifi
Copy link
Contributor

@bilal-alsharifi bilal-alsharifi commented Aug 23, 2019

Fixes #1157

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Test using ChoiceSetManager and make sure that all operations are handled correctly

Summary

This PR changes how the executor service handles operations in ChoiceSetManager to make the operations block until all async calls are completely finished. With this fix, the executor doesn't start executing the next operation until the current operation calls finishOperation()

CLA

@bilal-alsharifi bilal-alsharifi changed the title [WIP] Make ChoiceSetManager operations blocking Make ChoiceSetManager operations blocking Aug 27, 2019
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.

The test cases are not compiling for me.

@bilal-alsharifi bilal-alsharifi changed the title Make ChoiceSetManager operations blocking [WIP] Make ChoiceSetManager operations blocking Aug 29, 2019
@codecov-io
Copy link

codecov-io commented Aug 29, 2019

Codecov Report

Merging #1155 into feature/cancel_interaction_RPC will increase coverage by 3.16%.
The diff coverage is 48.23%.

Impacted file tree graph

@@                         Coverage Diff                          @@
##             feature/cancel_interaction_RPC    #1155      +/-   ##
====================================================================
+ Coverage                             43.99%   47.16%   +3.16%     
+ Complexity                             4259     4128     -131     
====================================================================
  Files                                   462      462              
  Lines                                 25416    22674    -2742     
  Branches                               3286     2577     -709     
====================================================================
- Hits                                  11183    10694     -489     
+ Misses                                13511    11324    -2187     
+ Partials                                722      656      -66
Impacted Files Coverage Δ Complexity Δ
...rs/screen/choiceset/PresentChoiceSetOperation.java 40.33% <16.66%> (-2.36%) 18 <0> (ø)
...gers/screen/choiceset/PreloadChoicesOperation.java 20% <20%> (-0.73%) 12 <1> (ø)
...agers/screen/choiceset/DeleteChoicesOperation.java 28.57% <20%> (-3.69%) 3 <1> (ø)
...anagers/screen/choiceset/BaseChoiceSetManager.java 39.31% <22.22%> (+1.26%) 24 <2> (-1) ⬇️
...ers/screen/choiceset/PresentKeyboardOperation.java 38.92% <37.5%> (-10.73%) 13 <1> (-1)
...reen/choiceset/CheckChoiceVROptionalOperation.java 27.27% <42.85%> (+0.6%) 6 <2> (ø) ⬇️
...nagers/screen/choiceset/AsynchronousOperation.java 93.33% <90%> (-1.67%) 14 <11> (+6)
...tdevicelink/managers/video/VideoStreamManager.java 38.73% <0%> (-6.21%) 13% <0%> (-1%)
...smartdevicelink/encoder/VirtualDisplayEncoder.java 24.43% <0%> (-4.55%) 8% <0%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7039e3...3b610a8. Read the comment docs.

@bilal-alsharifi
Copy link
Contributor Author

bilal-alsharifi commented Aug 29, 2019

@NicoleYarroch I updated the tests to make them compile. But then I had to comment out the cancel interactions tests temporarily. Because I noticed that in these tests, operation.run() is called to start the operation which makes the runnable run on the main thread. In real life, operations should run in a different thread (the executor service thread). I added a more detailed comment about that in the cancel interaction PR.
Because the operations run on the same thread in the tests, calling block() in the operation.run() function will block that main thread which causes the tests to get stuck and Travis to timeout.

I will keep these tests temporarily commented out until we decide how to handle operation canceling in the original cancel interaction PR.

@bilal-alsharifi bilal-alsharifi changed the title [WIP] Make ChoiceSetManager operations blocking Make ChoiceSetManager operations blocking Sep 11, 2019
@NicoleYarroch NicoleYarroch merged commit 70f343d into feature/cancel_interaction_RPC Sep 12, 2019
Tnnnguyen added a commit to Tnnnguyen/sdl_android that referenced this pull request Sep 13, 2019
* develop_mainRepo_local: (60 commits)
  fix is paused logic
  Make ChoiceSetManager operations blocking (smartdevicelink#1155)
  handle case where TM is null in protocolbase
  add a null check to proxylistener
  fix thread lock issue
  notify iProxyListenerBase of service ending
  remove deprecation and add javadoc
  fix incorrect method logic
  add one more check
  allow videos to be stopped properly with secondary transport disconnection
  fix older video stream restarts
  Removed unused code
  Refactored Thread.currentThread().isInterrupted()
  Removed uneeded deprecated presentKeyboard()
  dismissKeyboard() now takes a nonnull Integer
  Attempting to fix spacing issue in tests
  Update base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
  Update base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
  Update base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
  Update base/src/main/java/com/smartdevicelink/proxy/rpc/ScrollableMessage.java
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants