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

Use NSOperationQueue (GH-452) #454

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Use NSOperationQueue (GH-452) #454

wants to merge 2 commits into from

Conversation

ECNU3D
Copy link

@ECNU3D ECNU3D commented Nov 6, 2018

Platforms affected

iOS

What does this PR do?

A potential solution for issue #452

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@janpio
Copy link
Member

janpio commented Nov 6, 2018

Tests are failing. Can you please take a look at those?

@codecov-io
Copy link

Codecov Report

Merging #454 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #454   +/-   ##
=======================================
  Coverage   74.29%   74.29%           
=======================================
  Files          12       12           
  Lines        1564     1564           
=======================================
  Hits         1162     1162           
  Misses        402      402

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 7a006db...204dae4. Read the comment docs.

@ECNU3D
Copy link
Author

ECNU3D commented Nov 6, 2018

Fixed @janpio

@brodycj
Copy link
Contributor

brodycj commented Nov 12, 2018

I just updated the title to quickly reflect the actual proposed change. I would favor this change in general for the next major release, would like to see some testing according to https://github.com/apache/cordova-coho/blob/master/docs/platforms-release-process.md#testing and quick performance testing.

I cannot promise when any of us will have a chance to test and merge, suggest you continue to ask on [email protected].

@brodycj
Copy link
Contributor

brodycj commented Nov 12, 2018

P.S. If we merge this one I would like to see a similar change on cordova-osx (macOS), just raised apache/cordova-osx#70 for tracking.

@ECNU3D
Copy link
Author

ECNU3D commented Nov 15, 2018

@brodybits thanks, I will try to follow that document and post some plugin to demonstrate the issue.

@purplecabbage
Copy link
Contributor

Thank you @ECNU3D

I like this change, however there are a few things I would like to see here. ( all subject to discussion of course )

  1. The queue max size should be NSOperationQueueDefaultMaxConcurrentOperationCount, instead of arbitrarily choosing 32.
  2. The quality of service should also be set to default (NSQualityOfServiceDefault) instead of choosing to automatically run all commands low priority.
  3. We need developers to be aware that there are issues with their applications attempting too many calls in cases where the queue size is exceeded, or at least warn of potential issues. I am not sure exactly how I would approach this, something to discuss.
  4. I would like to see tests that demonstrate that we are not blocking and multiple short running commands can complete successfully while a long running command is also running.

It would be nice to know if there have ever been issues here, not sure how to approach this, but I dislike spending cycles fixing issues that have never affected anyone. Granted this is a small change, so I feel okay about this one regardless of proof that it ever happened. Just curious ...

@shazron
Copy link
Member

shazron commented Dec 3, 2018

Agree with @purplecabbage.
Also -- I'd rather not break things -- as a suggestion I would be comfortable if we feature flag it (a preference in config.xml?) and bake this in right away. BUT set this new implementation as default, with the flag to turn it off (--no-nsoperation-queue), just in case... (which we can remove later easily).

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

Successfully merging this pull request may close these issues.

6 participants