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

Breaking: Asynchronous message sending with async(iOS 13 required) #527

Merged
merged 11 commits into from
Jun 27, 2023

Conversation

philips77
Copy link
Member

This PR is the second attempt after #526 to implement async methods for sending messages to the mesh network.

This time message types were left alone, so the send(...) methods return generic MeshResponse or ConfigResponse types and user has to cast manually.

Apart from that:

  1. Minimum iOS version was set to 13.0 to support async/await.
  2. Minimum Swift version is 5.5.
  3. Messages are sent using a Task instead of DispatchQueue. The queue param in the MeshNetworkManager.init is now deprecated and unused.
  4. Callback-based send(..) methods use the async methods under the hood.
  5. It is possible to cancel sending a method using Task.cancel(), but the callback-based methods still return MessageHandle as they used to.

I did not migrate NetworkManager to be an actor. That will have to wait.

@eanderlind
Copy link

eanderlind commented Jun 22, 2023

A few higher-level comments to complement forthcoming in-file ones:

  1. Would be great to have async versions of MeshNetworkManager.send() etc. Only needed for the versions that have Sendable parameters (ie no class objects as parameters). Instead of passing Element, use Element.index or the element's address. Similarly for ApplicationKey can pass in AppKeyidx.
  2. Consider providing async and existing APIs in parallel to permit developers to gradually transition an implementation (or at least convert one part at a time, building and testing as go along).
  3. If use continuation under the hood for an async call, any errors reported by the callback should be converted to throws
  4. When wrapping a call in a Task, consider following syntax from Swift guide:
let handle = Task {
    return await add(newPhoto, toGalleryNamed: "Spring Adventures")
}
if foo { handle.cancel }  //my add
let result = await handle.value

I don't think the current MeshHandle.cancel() method will do the job correctly for an async Task.
5. TBD whether run task on current actor (Task {}), detached (Task.detached {}) or with TaskGroup. Suggest start simple, do some tests to see performance and whether propagating task cancellation is needed ,and update in a future pull request.

Copy link

@eanderlind eanderlind left a comment

Choose a reason for hiding this comment

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

See summary in immediately preceding comment.

@@ -182,25 +182,29 @@ internal class NetworkManager {
/// - initialTtl: The initial TTL (Time To Live) value of the message.
/// If `nil`, the default Node TTL will be used.
/// - applicationKey: The Application Key to sign the message.

Choose a reason for hiding this comment

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

Should document under what conditions the method throws

nRFMeshProvision/Layers/NetworkManager.swift Show resolved Hide resolved

private extension NetworkManager {

func setDeliveryCallback(for destination: Address,
Copy link

@eanderlind eanderlind Jun 22, 2023

Choose a reason for hiding this comment

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

These apis assume there is a single callback per destination. May need similar approach as in NetworkConnection where there is a stack of delegates.

Copy link
Member Author

Choose a reason for hiding this comment

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

The limit per single callback per destination comes from the mesh spec. A Node can send only one message to a single destination address at a time.

Choose a reason for hiding this comment

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

I wasn't clear in my comment. There can be >1 listener in App (UI plus e.g. the executeNow() function in ConfigurationViewController if permitted to continue executing after user navigates away, or some logging function) When add SwiftUI it is way easier if UIkit ViewControllers continue to pass delegate to each other and add a 2nd (model controller) listener for when a SwiftUI view is responsible for presenting errors to end-user.

}
}

func setResponseCallback(for request: AcknowledgedMeshMessage,

Choose a reason for hiding this comment

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

Difficult to predict/derive the response source address(es) when sending to a Group address. Only approach we have found to work is matching opcodes and all the msg parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

When an acknowledged message is sent to a Group address the callback will not await for responses (as 0+ of such is equally expected), but rather will be called upon delivery (that is when the request is sent).
The response callback is only send for acknowledged messages sent to a Unicast Address.

Choose a reason for hiding this comment

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

ok - I didn't realize that is what these functions were for. Thought it was some new capability for App to listen in to what is happening...

@@ -276,8 +265,8 @@ public extension MeshNetworkManager {
guard let networkManager = networkManager else {
return
}
queue.async {
networkManager.handle(incomingPdu: data, ofType: type)
Task {
Copy link

@eanderlind eanderlind Jun 22, 2023

Choose a reason for hiding this comment

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

Also provide an async version of bearerDidDeliverData() so that can evolve NetworkConnection?
Task.detached {} since typically switch actor?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually considering removing async from handle(incomingPdu:ofType:) in NetworkManager. It's not asynchronous anyway.
I'd just call it from Task.detached in MeshNetworkManager, but the bearerDidDeliverData can be non-blocking only.

Choose a reason for hiding this comment

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

Agree - ultimately want a synchronous call from NetworkConnection/BluetoothManager thread to quickly deposit incoming msg in a receive buffer so that thread/task waiting doesn't cause msg reordering. Unclear how to best architect this until start introducing actors (wouldn't want to block while waiting for a networking stack actor to complete something). Since AccessLayer is out of scope of this chg, didn't make the comment yesterday.

@@ -310,7 +299,7 @@ public extension MeshNetworkManager {
let _ = meshNetwork?.applicationKeys[publish.index] else {
return nil
}
queue.async {
Task {
networkManager.publish(message, from: model)

Choose a reason for hiding this comment

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

May need a revised .publish api. I don't think can capture and pass in Model class to Task. May depend on XCode version what warnings are generated (the purple ones...) and which Swift version it becomes an error.
I may have this mixed up with requirements for Actors.....

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave it for now like that. I have no warnings.
If we see issues later, this can be refactored.

func send(_ message: MeshMessage,
from localElement: Element? = nil, to destination: MeshAddress,
withTtl initialTtl: UInt8? = nil,
using applicationKey: ApplicationKey,
completion: ((Result<Void, Error>) -> ())? = nil) throws -> MessageHandle {
using applicationKey: ApplicationKey) async throws {

Choose a reason for hiding this comment

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

may need api of form
func send(message: MeshMessage, localElementIdx: UInt8?=nil, destination: MeshAddress, initialTtl:, applicationKeyIdx: KeyIndex) async throws {
such that synchronous code that makes the call from within a new Task where the task captures required Sendable method parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

func send(_ message: UnacknowledgedConfigMessage, to destination: Address,
withTtl initialTtl: UInt8? = nil,
completion: ((Result<Void, Error>) -> ())? = nil) throws -> MessageHandle {
withTtl initialTtl: UInt8? = nil) async throws {

Choose a reason for hiding this comment

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

As above, may need revised api with Sendable paramteters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like above, perhaps later. So far it works for me without warnings.

@@ -819,8 +763,8 @@ public extension MeshNetworkManager {
print("Error: Mesh Network not created")
throw MeshNetworkError.noNetwork
}
queue.async {
networkManager.send(message)
Task {

Choose a reason for hiding this comment

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

Provide an async variant of the method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Later perhaps.

Base automatically changed from message-callbacks to main June 27, 2023 11:01
@philips77 philips77 changed the title Asynchronous message sending - with async-first approach Breaking: Asynchronous message sending with async(iOS 13 required) Jun 27, 2023
@philips77 philips77 merged commit 2876660 into main Jun 27, 2023
@philips77 philips77 deleted the message-callbacks-async branch June 27, 2023 13:52
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.

2 participants