Define commands as asynchronous and use Task for preview cancellation

[d-ronnqvist]

Bug/issue #, if applicable:

Summary

This is the third of many changes to redefine how documentation contexts are created.

This redefined the various commands to be asynchronous so that preview action cancellation, documentation converter cancellation, and context registration cancellation can use Task.checkCancellation() instead of sharing a Boolean wrapped in a class and polling for cancellation on a timer.

This change also supports cancelling context registration during initialization, which enables us to move away from deferred context registration in a follow up PR.

Dependencies

This builds on top of #1052

Testing

Nothing in particular. This isn't a user-facing change.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added Updated tests
• Ran the ./bin/test script and it succeeded
• [ ] Updated documentation if necessary

[d-ronnqvist]

How much do we care about source compatibility for the command line actions? I find that I need to write literal anti-patterns to make it compatible and I don't know if there's an expectation that that code should be used by anyone outside of this package. Should we stop surfacing it as a "product"?

[d-ronnqvist]

How much do we care about source compatibility for the command line actions? I find that I need to write literal anti-patterns to make it compatible and I don't know if there's an expectation that that code should be used by anyone outside of this package. Should we stop surfacing it as a "product"?

I made this new commit to stop surfacing the command line actions as a library product. This would be source breaking since consumers of this repo no longer can import it.

[d-ronnqvist]

How much do we care about source compatibility for the command line actions? I find that I need to write literal anti-patterns to make it compatible and I don't know if there's an expectation that that code should be used by anyone outside of this package. Should we stop surfacing it as a "product"?

I made this new commit to stop surfacing the command line actions as a library product. This would be source breaking since consumers of this repo no longer can import it.

I moved that change to #1052 instead.

[patshaughnessy]

Looks great.

Are there any implications for switching to a async functions throughout the CLI and top level action structs?

Is there a way we can refactor the code in DocumentationContext.register to use async notation to manage the various discovery tasks, instead of using DispatchGroup and DispatchQueue?

[patshaughnessy]

Why do we need isCancelled ? I don't see code that sets it anywhere. Is calling Task.isCanclled sufficient?

[d-ronnqvist]

DocumentationConverter is public API it's possible that a caller is passing a shared isCancelled during initialization and setting it to true to cancel the conversion.

[patshaughnessy]

Do we ever use willPerformFuture ? Or didPerformFuture ? Can we remove them?

[d-ronnqvist]

Only in tests. PreviewActionIntegrationTests.testCancelsConversion() sets a willPerformFuture that sleeps to artificially slow down the conversation time to a controllable number to give the test time to modify a monitored file to trigger another conversion which cancels the previous one.

[d-ronnqvist]

Only willPerformFuture was ever used so I renamed it to _extraTestWork to better indicate what it is and removed didPerformFuture.

[d-ronnqvist]

Are there any implications for switching to a async functions throughout the CLI and top level action structs?

Not really since they're top-level. The implication of switching to asynchronous functions is that they can only be called from other asynchronous functions (or main). This implication can be seen in all the tests that needed to add await.

This problem is illustrated in the first half of What Color is Your Function?. Since asynchronous functions are able to call both asynchronous and synchronous functions but synchronous functions are only able call other synchronous functions, asynchronous functions spread outwards. Because of this, it's sometimes recommended to make updates like this from the outside it.

Is there a way we can refactor the code in DocumentationContext.register to use async notation to manage the various discovery tasks, instead of using DispatchGroup and DispatchQueue?

Yes, but it's out of scope for this PR.

Using asynchronous functions in DocumentationContext.register would require us to make it an asynchronous function which would mean that the places that its callers (DocumentationContext.init(dataProvider:diagnosticEngine:configuration:) and DocumentationContextDataProviderDelegate.dataProvider(_:didAddBundle:)) would need to become asynchronous functions which would be source breaking changes (since the caller would need to await their results and the caller would need to become asynchronous as well).

In the fourth PR in this series—or possibly fifth PR depending on how I split up the changes—I plan on deprecating both those callers and introducing a new context initializer which will be asynchronous even though it only calls synchronous code. This will open up to a future where we can make DocumentationContext.register asynchronous, either by waiting until we can remove the deprecated synchronous callers or by maintaining two separate versions (one using DispatchGroup and one using Task and async/await)

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

@swift-ci please test

[d-ronnqvist]

I noticed that the updated tests were flaky because of their use of wait(for:timeout:enforceOrder:) instead of the Concurrency safe alternative fulfillment(of:timeout:enforceOrder:).

While I was investigating that I also found and fixed a couple of memory leaks.

[d-ronnqvist]

@swift-ci please test

[patshaughnessy]

In the fourth PR in this series—or possibly fifth PR depending on how I split up the changes—I plan on deprecating both those callers and introducing a new context initializer which will be asynchronous even though it only calls synchronous code.

This sounds great - looking forward to seeing that!