Use NSOperationQueue (GH-452)

[ECNU3D]

Platforms affected

iOS

What does this PR do?

A potential solution for issue #452

What testing has been done on this change?

Checklist

• [x] Reported an issue in the JIRA database
• [x] 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.
• [x] Added automated test coverage as appropriate for this change.

[janpio]

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

[codecov-io]

Codecov Report

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

@@           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]

Fixed @janpio

[brody4hire]

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].

[brody4hire]

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

[ECNU3D]

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

[purplecabbage]

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]

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).

[erisu]

Closing PR as stale and has conflicts.

It appears that it could have been accepted but there were no actions taken after the feedback.

If there is any desire to get this though, I suggest opening a new PR to be implemented on the current main branch, make necessary changes based on the previous feedback.

Will keep open the original issue ticket.